Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding app_context from SenNet #1299

Merged
merged 6 commits into from
Feb 20, 2024
Merged

Adding app_context from SenNet #1299

merged 6 commits into from
Feb 20, 2024

Conversation

gesinaphillips
Copy link
Collaborator

@gesinaphillips gesinaphillips commented Feb 15, 2024

This deviates from the SenNet in a few necessary and one maybe-tryhard way.

  • Necessary: I need to enhance the update in SenNet to allow for passing the Ingest API URL as well. This led to the changes in validation_utils.
  • Am I doing too much here?: I added Upload.get_app_context to clean the submitted app_context data to ensure that all necessary keys/values are present, but to prioritize any user-submitted data.

Note the TODO in upload.py regarding the hardcoded defaults.

    # TODO: this would be better in a config so it could be shared with
    # validation_utils.get_assaytype_data / would not conflict with SenNet values
    # For the moment, ingest_url is just hard-coded in get_assaytype_data
    # to allow validate_tsv.py to run

What do you think?

@gesinaphillips gesinaphillips marked this pull request as ready for review February 15, 2024 17:00
@gesinaphillips gesinaphillips requested review from sunset666 and jpuerto-psc and removed request for jpuerto-psc February 15, 2024 17:00
@jpuerto-psc
Copy link
Collaborator

@gesinaphillips - so the hard-coding is setting the ingest-api value when the validate_tsv function is used outside of the context of an upload? I think this will cause issues in SenNet (not sure) because I believe that they do exactly that. They call that function without constructing an upload, when someone uploads a metadata.tsv file. So I think we would have to hard-code that value in the SenNet IVT. Everything else looks fine!

@gesinaphillips
Copy link
Collaborator Author

@jpuerto-psc validation_utils.get_tsv_errors actually ends up creating an Upload (kind of a strange pattern, still trying to think of an alternative). It's actually the validation_utils.get_schema_version call in validate_tsv.py where the ingest_url is required, which is why it's hard-coded directly into validation_utils.get_assaytype_data (which is called by get_schema_version).

But either way, if we have the HuBMAP URLs hard-coded in upload.py and validation_utils.py, they will have to make sure to change those to add their own SenNet URLs.

@sunset666
Copy link
Collaborator

sunset666 commented Feb 15, 2024

@jpuerto-psc, Updated: Not a major issue with the update on SenNet since the fork will allow the hardcoded url to be updated, we just need to inform about this.

@jpuerto-psc
Copy link
Collaborator

@jpuerto-psc validation_utils.get_tsv_errors actually ends up creating an Upload (kind of a strange pattern, still trying to think of an alternative). It's actually the validation_utils.get_schema_version call in validate_tsv.py where the ingest_url is required, which is why it's hard-coded directly into validation_utils.get_assaytype_data (which is called by get_schema_version).

But either way, if we have the HuBMAP URLs hard-coded in upload.py and validation_utils.py, they will have to make sure to change those to add their own SenNet URLs.

Fantastic, then we're good here I think!

@gesinaphillips gesinaphillips merged commit b726cb3 into main Feb 20, 2024
8 checks passed
@gesinaphillips gesinaphillips deleted the phillips/app_context branch February 20, 2024 15:04
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.

3 participants