Skip to content

Conversation

@jlchang
Copy link
Contributor

@jlchang jlchang commented Oct 12, 2021

Tests in single_cell_portal repo fail due to Classes in the repo that were refactored earlier this year. Methods used for tests in single_cell_portal are now obsolete. To use scp-ingest-pipeline as-is in the tests, an IngestPipeline object would need to be instantiated to use the conforms_to_metadata_convention method, which is more complex than instantiating a CellMetadata object which is what the current, broken tests do. Logically, conforms_to_metadata_convention should be a CellMetadata method because none of the other ingest file types require the metadata convention. To ease the repair of tests in the single_cell_portal repo, this PR moves the conforms_to_metadata_convention method to the Cell Metadata Class.

To test (is kinda finicky right now, sorry!)
pre-test: find a study in your dev instance and get a valid study_id and study_file_id
ensure your python environment is up to date with requirements.txt in scp-ingest-pipeline repo
(not sure if this works differently if your gcloud config is set to your service account or your Broad identity, I used my Broad identity)
Have your mondoDB credentials set up as environment variables (Let me know if this isn't documented already, I think Eno made a doc somewhere. I have a script that set's it up for me using vault commands)

Run the following
python ingest_pipeline.py --study-id --study-file-id ingest_cell_metadata --cell-metadata-file <path/to/repo>/scp-ingest-pipeline/tests/data/annotation/metadata/convention/valid_array_v2.1.2.txt --study-accession --ingest-cell-metadata --validate-convention

Expect many warning messages but no errors.

This work supports SCP-3631 and corrects the import errors that blocked SCP-3414.

Copy link
Member

@eweitz eweitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

setup.py Outdated
setup(
name="scp-ingest-pipeline",
version="1.11.0",
version="1.11.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current version is 1.12.0, so this should be higher than that.

Copy link
Contributor Author

@jlchang jlchang Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch. I had incremented to test using TestPyPI. I forgot that I needed to update it accurately (PyPI won't let you use a version tag twice and I was afraid I'd accidentally mess up 1.12.1 in my testing!)


def conforms_to_metadata_convention(self):
""" Determines if cell metadata file follows metadata convention"""
convention_file_object = IngestFiles(self.JSON_CONVENTION, ["application/json"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope, but it'd be nice if we define default MIME types for given extensions (e.g. ("json": ["application/json"]) in only one place, so we could typically omit second parameters like this ["application/json"].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it would be nice to do.

@eweitz eweitz changed the title Relocate IngestPipeline method about metadata convention to CellMetadata Class (SCP3632) Relocate IngestPipeline method about metadata convention to CellMetadata Class (SCP-3632) Oct 12, 2021
@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #216 (f6fff85) into development (1b4d9ee) will decrease coverage by 0.14%.
The diff coverage is 40.90%.

❗ Current head f6fff85 differs from pull request most recent head a32f8aa. Consider uploading reports for the commit a32f8aa to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development     #216      +/-   ##
===============================================
- Coverage        71.44%   71.29%   -0.15%     
===============================================
  Files               26       26              
  Lines             3089     3101      +12     
===============================================
+ Hits              2207     2211       +4     
- Misses             882      890       +8     
Impacted Files Coverage Δ
ingest/__init__.py 0.00% <0.00%> (ø)
ingest/expression_files/mtx.py 93.81% <0.00%> (ø)
ingest/ingest_pipeline.py 61.88% <0.00%> (ø)
ingest/cell_metadata.py 74.15% <37.50%> (-7.18%) ⬇️
ingest/validation/validate_metadata.py 77.42% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b4d9ee...a32f8aa. Read the comment docs.

Copy link
Contributor

@bistline bistline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for figuring this out finally!

jlchang and others added 2 commits October 13, 2021 08:06
@jlchang jlchang merged commit de22060 into development Oct 13, 2021
@jlchang jlchang deleted the jlc_move_conform_method branch October 13, 2021 16:49
@jlchang jlchang changed the title Relocate IngestPipeline method about metadata convention to CellMetadata Class (SCP-3632) Relocate IngestPipeline method about metadata convention to CellMetadata Class (SCP-3631) Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants