Harden Genomes Pipeline and make_toy_data.py (SCP-2491) #129
Conversation
Quick local test yields 1.5 minutes parallelized vs 2.5 minutes serial, i.e. 1 minute or 40% faster
eweitz
requested review from
bistline,
devonbush,
jlchang and
knapii-developments
eweitz
changed the title
|
Oddly, even though I used e.g. The affected histories are short, as those files were recently migrated from another repo. The diffs would also be large even with proper |
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
===========================================
+ Coverage 51.82% 67.00% +15.18%
===========================================
Files 22 22
Lines 2580 2576 -4
===========================================
+ Hits 1337 1726 +389
+ Misses 1243 850 -393
Continue to review full report at Codecov.
|
| To use `ingest/genomes/genomes_pipeline.py`: | ||
|
|
||
| ``` | ||
| brew install tabix |
There was a problem hiding this comment.
Glad we have this documented now!
devonbush
left a comment
devonbush
left a comment
There was a problem hiding this comment.
overall looks good, just some style questions
| remote_prod_dir = config['remote_prod_dir'] | ||
|
|
||
| storage_client = get_gcs_storage_client(vault_path) | ||
| storage_client = utils.get_gcs_storage_client(vault_path) |
There was a problem hiding this comment.
nice, I like how you made these easier to read where the functions come from
| new_metadata_ref = [] | ||
|
|
||
| ref_file = 'species_metadata_reference.tsv' | ||
| ref_file = f'{output_dir}species_metadata_reference.tsv' |
There was a problem hiding this comment.
is this a place where os.path.join might be more robust?
https://docs.python.org/3/library/os.path.html
There was a problem hiding this comment.
Yup! Done in 8a486fe. It's more verbose, but trading that for robustness makes sense.
tests/test_genomes.py
Outdated
| # Run all tests, show code coverage metrics | ||
| pytest --cov=../ingest/ | ||
There was a problem hiding this comment.
I think it's overkill to be repeating pytest usage documentation here. Maybe just add a link to the pytest docs from the main readme (line 76ish)?
There was a problem hiding this comment.
Yeah, I can see how repeating these examples in each integration test module is a bit much. At the same time, I think only linking to pytest docs makes conveying tailored examples too inconvenient.
So in 8a486fe I've consolidated this handful of conveniences into the "Test" section of the README, added a pointer here, and removed the examples from the tests themselves.
tests/test_make_toy.py
Outdated
| pytest test_make_toy.py | ||
| # Run all tests, show code coverage metrics | ||
| pytest --cov=../ingest/ |
There was a problem hiding this comment.
same, I don't think it's useful to repeat pytest docs at the top of every test
There was a problem hiding this comment.
Agreed; details are in my reply over there.
This improves robustness and maintainability for the SCP Genomes Pipeline and make_toy_data.py.
Previously, these two pieces of infrastructure had no automated tests, and a tedious complex of parameters. The code had moved from another repo in #102 to reduce technical debt after gaining significant features, but those two liabilities remained.
Now those risks are greatly reduced. Genomes Pipeline (
genomes_pipeline.py, previously namedprocess_genome_references.py) now has an integration test intest_genomes.pyandmake_toy_data.pyhas one intest_toy_data.py. To enable tests, the code has been restructured to enable use as a Python module. Altogether, this substantially increasesscp-ingest-pipelinetest coverage -- from 52% before to 67% after.Also, Genomes Pipeline is now more object-oriented, to simplify stateful transformations of genome reference data. Static analysis via flake8 has also been restored for those files. These two changes improve maintainability for four modules comprising ~700 lines of code, which is about a quarter of the code in
scp-ingest-pipeline.While manually verifying this work, I discovered that reference data can no longer be uploaded to GCS due to network timeouts. This seems to be due to slower networks, and independent of these changes. SCP-2662 tracks that.
This satisfies SCP-2491.