refactor: create antismash downloader module and move there inherent code #119

gcroci2 · 2023-02-28T15:14:39Z

I moved popd antismash download and extraction related stuff to nplinker/pairedomics/
I moved the code regarding antismash data download and extraction from pairedomics/downloader.py to genomics/antismash/downloader.py. What about using only downloader.py as the module name? It's already in antismash/ folder, so calling it antismash_downloader.py seems redundant to me.
I refactored old function download_antismash_data, which now is deprecated, and created a new one download_and_extract_antismash_data, which takes as input a single item id to be used to retrieve Antismash data. In the refactored downloader for PoDP it will be inserted in a for loop to iterate over all the items available. The downloader module will be refactored in a different PR (see issue Refactor Downloader class to have a specific PoDP downloader #121).
I moved antismash tests to tests/antismash folder, calling them test_downloader.py and test_loader.py, to not repeat myself. But this is related to the comment above, so let me know if we want to keep antismash_[...].py in the naming or if we want to remove that because everything is already in antismash/ folders.
I've run prospector and static typing checks, and reordered imports on the new antismash files
I've added a proper docstring to the antismash downloader file

Notes

Two tests are failing in the dev branch, and thus also here: tests/test_nplinker.py::test_get_links and pytest tests/test_nplinker.py::test_load_data; it would be handy to fix them before merging this PR, just to be sure that it doesn't introduce any issues. In both cases, the error given is TypeError: unhashable type: 'Strain'. Should I open a new issue about the failing tests? Something similar but different was happening in the same tests (see issue Fix failing tests #96), but should have been already solved.

CunliangGeng

The antismash_downloader.py handles with the downloading of antismash data based on PODP input. If the input changes, then it's still not easy to use this downloader. We need to further refactor this part, and let's have a chat about it tomorrow.

src/nplinker/genomics/antismash/antismash_downloader.py

Co-authored-by: Cunliang Geng <[email protected]>

…nker/nplinker into 98_add_antismash_downloader_gcroci2

src/nplinker/genomics/antismash/antismash_downloader.py

hechth · 2023-03-08T07:56:00Z

@gcroci2 did you branch from the most up-to-date version of dev? I introduced the hashing function for strains in #118 I think.

src/nplinker/genomics/antismash/antismash_downloader.py

src/nplinker/pairedomics/podp_antismash_downloader.py

gcroci2 · 2023-03-08T10:33:41Z

@gcroci2 did you branch from the most up-to-date version of dev? I introduced the hashing function for strains in #118 I think.

I've just merged dev branch :)

gcroci2 · 2023-03-09T10:30:08Z

tests/genomics/antismash/test_downloader.py

+import pytest
+from nplinker.genomics.antismash import download_and_extract_antismash_metadata
+
+class TestDownloadAndExtractAntismashData():


I would like to implement a fixture method for use download_root, extract_root, and refseq_assembly_id across the entire class. I tried to look it up online but I don't get how to do it for multiple variables.

Apparently, passing in tmp_path creates a default testing dir which is deleted at the end of the test. Is this the best practice though? Would be nice to create temporary folders using tmpdir in a fixture method, and then retain the temp folders across the entire class. How should I implement that?

For refseq_assembly_id you can directly use refseq_assembly_id="GCF_004339725.1" as a global variable in the class, then you can directly use the variable in functions.

If it's really necessary to share the folders across class, you can see the example using fixture tmp_path_factory: https://pytest.org/en/latest/how-to/tmp_path.html#the-tmp-path-factory-fixture

I've read the docs there already, but if I try to implement it by creating a temporary folder and yielding it, then the folder is unknown to the other methods of the class.

Can the tmp_dir be a class attribute? So that the whole class just has one link to that which is created when parsing the class?

Indeed, this would be the alternative. But I was looking for using the fixture thing. Anyway it was more for my personal curiosity, it's not needed at all in this specific test :)

gcroci2

See my comment on the top of the PR @CunliangGeng

CunliangGeng · 2023-03-09T14:51:13Z

See my comment on the top of the PR @CunliangGeng

@gcroci2 The failed tests have been solved and the latest commit of dev branch passed all tests.
Please give me a signal when the PR is ready to review again.

gcroci2 · 2023-03-09T16:31:30Z

See my comment on the top of the PR @CunliangGeng

@gcroci2 The failed tests have been solved and the latest commit of dev branch passed all tests. Please give me a signal when the PR is ready to review again.

I've merged with dev but they're still failing on my local machine, different error now:

=========================================================== short test summary info ===========================================================
FAILED tests/test_nplinker.py::test_load_data - assert 364 == 390
FAILED tests/test_nplinker.py::test_get_links - assert 58 == 101

CunliangGeng · 2023-03-09T16:59:10Z

FAILED tests/test_nplinker.py::test_load_data - assert 364 == 390
FAILED tests/test_nplinker.py::test_get_links - assert 58 == 101

@gcroci2 The branch of this PR passed all tests on my laptop. So there must be something wrong with your data. You could remove all data in ~/nplinker_data folder but keep bigscape data (to save time), and then run the test again.

hechth · 2023-03-10T07:28:28Z

FAILED tests/test_nplinker.py::test_load_data - assert 364 == 390
FAILED tests/test_nplinker.py::test_get_links - assert 58 == 101

@gcroci2 The branch of this PR passed all tests on my laptop. So there must be something wrong with your data. You could remove all data in ~/nplinker_data folder but keep bigscape data (to save time), and then run the test again.

Maybe you ran an older version of bigscape? might also be possible :/

CunliangGeng

The last commit "xMerge branch 'dev' into 98_add_antismash_downloader_gcroci2
" is wrong and it removed all the latest updates of dev branch.

Please revert this commit, and make sure the file changes of this PR only involves the target files (for this PR they should only be downloader files).

This reverts commit f64a17c, reversing changes made to 4879181. revert merge

…croci2"" This reverts commit 0c5b6bd. revert last merge

gcroci2 · 2023-03-15T16:39:15Z

Because of reverting merged commit issues, I've opened a new PR (#127)

creat antismash downloader module

8f64998

gcroci2 linked an issue Feb 28, 2023 that may be closed by this pull request

Create antismash_downloader module #98

Closed

4 tasks

gcroci2 self-assigned this Feb 28, 2023

gcroci2 added the GEN genomics related issues label Feb 28, 2023

gcroci2 added 2 commits February 28, 2023 16:23

fix tests

ec2b048

Merge branch 'dev' into 98_add_antismash_downloader_gcroci2

d49f5fc

gcroci2 requested a review from CunliangGeng February 28, 2023 15:47

CunliangGeng requested changes Mar 1, 2023

View reviewed changes

CunliangGeng reviewed Mar 1, 2023

View reviewed changes

src/nplinker/genomics/antismash/antismash_downloader.py Show resolved Hide resolved

gcroci2 and others added 3 commits March 3, 2023 10:33

Update src/nplinker/genomics/antismash/antismash_downloader.py

26e685f

Co-authored-by: Cunliang Geng <[email protected]>

add a new function for downloading and extracting antismash data

da904c6

Merge branch '98_add_antismash_downloader_gcroci2' of github.com:NPLi…

8be7e44

…nker/nplinker into 98_add_antismash_downloader_gcroci2

gcroci2 requested a review from CunliangGeng March 3, 2023 16:52

gcroci2 commented Mar 3, 2023

View reviewed changes

src/nplinker/genomics/antismash/antismash_downloader.py Outdated Show resolved Hide resolved

CunliangGeng requested changes Mar 7, 2023

View reviewed changes

src/nplinker/genomics/antismash/antismash_downloader.py Outdated Show resolved Hide resolved

gcroci2 added 2 commits March 7, 2023 14:34

create podp_antismash_downloader module

7b07a7b

properly define download_and_extract_antismash_metadata function

4879181

hechth approved these changes Mar 8, 2023

View reviewed changes

src/nplinker/genomics/antismash/antismash_downloader.py Outdated Show resolved Hide resolved

src/nplinker/pairedomics/podp_antismash_downloader.py Show resolved Hide resolved

Merge branch 'dev' into 98_add_antismash_downloader_gcroci2

f64a17c

gcroci2 added 3 commits March 8, 2023 12:17

add internal funcs and doc string

392a736

add tests and create antismash test folder

850b746

format properly extract_path

e186ba8

gcroci2 commented Mar 9, 2023

View reviewed changes

gcroci2 added 2 commits March 9, 2023 11:40

run linting and formatting for modified files using yapf

61e17fe

fix prospector errors

601957d

gcroci2 commented Mar 9, 2023

View reviewed changes

make refseq_assembly_id class variable for tests

e9fbe99

gcroci2 added 2 commits March 9, 2023 13:35

reorder imports

9a22c06

add minor static typing

decf286

xMerge branch 'dev' into 98_add_antismash_downloader_gcroci2

e55fb33

gcroci2 requested a review from CunliangGeng March 9, 2023 16:31

CunliangGeng requested changes Mar 10, 2023

View reviewed changes

gcroci2 added 2 commits March 15, 2023 12:47

Revert "Merge branch 'dev' into 98_add_antismash_downloader_gcroci2"

0c5b6bd

This reverts commit f64a17c, reversing changes made to 4879181. revert merge

Revert "Revert "Merge branch 'dev' into 98_add_antismash_downloader_g…

c66a10a

…croci2"" This reverts commit 0c5b6bd. revert last merge

gcroci2 closed this Mar 15, 2023

gcroci2 deleted the 98_add_antismash_downloader_gcroci2 branch March 15, 2023 16:39

CunliangGeng mentioned this pull request Mar 31, 2023

Refactor Downloader class to have a specific PoDP downloader #121

Closed

5 tasks

gcroci2 mentioned this pull request Apr 6, 2023

Use the refactored antismash downloader in PODPDownloader class #137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: create antismash downloader module and move there inherent code #119

refactor: create antismash downloader module and move there inherent code #119

gcroci2 commented Feb 28, 2023 •

edited

Loading

CunliangGeng left a comment

hechth commented Mar 8, 2023

gcroci2 commented Mar 8, 2023

gcroci2 Mar 9, 2023 •

edited

Loading

CunliangGeng Mar 9, 2023

gcroci2 Mar 9, 2023

hechth Mar 10, 2023

gcroci2 Mar 15, 2023

gcroci2 left a comment •

edited

Loading

CunliangGeng commented Mar 9, 2023

gcroci2 commented Mar 9, 2023

CunliangGeng commented Mar 9, 2023

hechth commented Mar 10, 2023

CunliangGeng left a comment

gcroci2 commented Mar 15, 2023

refactor: create antismash downloader module and move there inherent code #119

refactor: create antismash downloader module and move there inherent code #119

Conversation

gcroci2 commented Feb 28, 2023 • edited Loading

CunliangGeng left a comment

Choose a reason for hiding this comment

hechth commented Mar 8, 2023

gcroci2 commented Mar 8, 2023

gcroci2 Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

CunliangGeng Mar 9, 2023

Choose a reason for hiding this comment

gcroci2 Mar 9, 2023

Choose a reason for hiding this comment

hechth Mar 10, 2023

Choose a reason for hiding this comment

gcroci2 Mar 15, 2023

Choose a reason for hiding this comment

gcroci2 left a comment • edited Loading

Choose a reason for hiding this comment

CunliangGeng commented Mar 9, 2023

gcroci2 commented Mar 9, 2023

CunliangGeng commented Mar 9, 2023

hechth commented Mar 10, 2023

CunliangGeng left a comment

Choose a reason for hiding this comment

gcroci2 commented Mar 15, 2023

gcroci2 commented Feb 28, 2023 •

edited

Loading

gcroci2 Mar 9, 2023 •

edited

Loading

gcroci2 left a comment •

edited

Loading