Make autocomplete_metadata.py emit schema-compliant molecule metadata and improve CI tests - #506
Conversation
autocomplete_metadata.py emit schema-compliant molecule metadata
There was a problem hiding this comment.
Pull request overview
This PR hardens developer/autocomplete_metadata.py so it generates metadata.yaml that conforms to src/fairmd/lipids/schema_validation/schema/metadata_schema.json, even when upstream API payloads contain dirty/HTML-ish strings or non-numeric weights. It also adds a regression test to validate the generated YAML against the JSON schema.
Changes:
- Added normalization helpers (
clean_text,safe_float) and applied them to schema-facing fields (names, SMILES, InChI/InChIKey, molecular weight). - Added
sanitize_sameasto keepsameAsidentifiers schema-pattern compliant. - Added
tests/test_autocomplete_metadata.pyto mock API responses and assert schema validation passes and cleaned fields match expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
developer/autocomplete_metadata.py |
Adds sanitization/parsing helpers and applies them to emitted metadata fields to improve schema compliance. |
tests/test_autocomplete_metadata.py |
Adds a mocked end-to-end autocomplete test that validates generated YAML against metadata_schema.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Added an option for cas retrieval from CAS commonchemistry.cas.org Needs an API key in env CAS_API_KEY
|
Have tested and debugged this. Now the output is formatted correctly. Also added a test. |
|
Currently test test_calc_ff_quality_sin_exp_curve is failling (on MacOS 15). Nothing to do with my changes. |
korbinib
left a comment
There was a problem hiding this comment.
looks good to me. Do we want to have the additional cas API call?
There was a problem hiding this comment.
It's good that you decided to add out-of-package tests, but then they must be clearly isolated from the package tests.
- We need to make sure they are not running on release or other stories when we distribute the package. Because
developfolder is not distributed with the package. It's conditioning stuff in the repo. - We need to name this kind of tests
develop, and notnetwork. There are other tests that use direct internet access and that are run currently insidetests-alltox label. - Tests
developtests should be accessible viatox. Currently, there is no tox label for them. And we should probably separate tests-all.
What is my suggestion.
- 1. Let's remove
tests-allenvironment and replace it withtests-packageenvironment. - 2. Let's add
tests-developenvironment. - 3. Let's call proper tox labels from proper events.
@MagnusSletten was working a lot with this part. I would like to also ask his opinion.
- Added develop pytest marker; mark test_autocomplete_metadata.py module-level - Replace tests-all tox env with tests-package (to be run: pytest -m "not develop") - Addd tests-develop tox env (run: pytest -m develop --no-cov) to avoid coverage warnings - Updated All Tests wf to run tests-package and add tests-develop
|
I added tests-package and replaced tests-all by it. Also added tests-develop. I am not sure if all events:call assignments are proper yet. Possibly need to discuss that. |
|
@mdondrup thanks. Now I cleaned CI a bit. I also would like to separate package and development tests and hang trigger to changes of different folders in PR. This is important I think.. |
|
// I don't like that we had different coverage reporting from integration and unit-tests. I would like to have only coverage from unit tests because integration tests doesn't help do develop, so I'm not very interested in having this value despite it's large. |
MagnusSletten
left a comment
There was a problem hiding this comment.
Nice pr!
Separating developer tests also means we can run them specifically when the developer/ folder is changed in CI, while keeping them isolated from the normal package tests.
Small nitpick: I slightly prefer keeping Codecov/coverage reporting in CI rather than baking it into tests-min, but I do not think this is a blocker. The generated files are gitignored, and this will not affect normal development at all.
Co-authored-by: MagnusSletten <magnus.elias.sletten@gmail.com>
Co-authored-by: MagnusSletten <magnus.elias.sletten@gmail.com>
comcon1
left a comment
There was a problem hiding this comment.
Now, we are ready! Thanks, all!
|
This PR has diverged from its original title and task. We should change the name and description. |
autocomplete_metadata.py emit schema-compliant molecule metadataautocomplete_metadata.py emit schema-compliant molecule metadata and improve CI tests
|
I think this was merged prematurely. Now the tests are not working |
autocomplete_metadata.pycould emit invalidmetadata.yamlentries (notably malformed strings/identifiers and invalidNMRlipidsfield types), causing schema validation failures for newly created molecule metadata. This update hardens output normalization so autogenerated metadata conforms tometadata_schema.jsonunder partial/dirty upstream API payloads.Output sanitization for schema-facing fields
clean_text) to strip HTML tags/entities from API-derived values before writingbioschema_propertiesand alternate names.safe_float) formolecularWeightto avoid invalid numeric output.sameAsidentifier hardeningsanitize_sameasto keep only schema-valid identifier values/patterns (e.g.,ChEBI,ChEMBL,cas,slm,pubchem.compoundas int).NMRlipidsschema complianceNMRlipids.nameis always populated via fallback chain (existing value → API names → molecule id).charge(schema expects integer when present).Focused regression coverage
tests/test_autocomplete_metadata.pyto execute the autocomplete flow with mocked API responses and assert generated YAML validates againstmetadata_schema.json, including cleanedsmiles/alternateName.-Clena up test environments and CI
During fixing, it was discovered that the tox test environments should be restructured.
See #506 (review) below