fix(json-schema): faithful attribute descriptions + panel-ID doc clarification - #196
Conversation
…age notes Fixes #191 (and #190 as a consequence). The flat Synapse JSON generator emitted induced-slot descriptions, so a class slot_usage carrying a conditional note (e.g. 'Required when X is Y') replaced the real attribute description in the output — affecting ~50 conditional attributes across Biospecimen and Clinical, which feed the BigQuery tabular release. Add restore_base_attribute_descriptions(): after generation, override each property description with the canonical attribute-level description (walking the class and its ancestors, ignoring slot_usage). The conditional requirement itself is still enforced by the generated rules (if/then) constraints; only the human-readable description changes. - Biospecimen SECTION_THICKNESS_VALUE now shows '...measured in microns' (#190) - ACQUISITION_METHOD_OTHER_SPECIFY now shows 'A custom acquisition method' (#191) - descriptions where the note IS the canonical text (e.g. scATAC AVERAGE_INSERT_SIZE) are unaffected; modules without slot_usage overrides regenerate identically - regression tests added in tests/test_linkml_schema_conversion.py
There was a problem hiding this comment.
HTAN Schema Review — ✅ Approved
Automated review by htan-claude · commit
e4c603a23ff5adbcf2e976e09af6da9bdb928f5f
This is a clean, well-scoped PR — all three reviewers came back with no issues, and the one informational note from the coverage reviewer is just good housekeeping context.
Tip
All checklist items pass — no issues found.
Files Changed
scripts/linkml_to_flat_synapse_jsonschema.pytests/test_linkml_schema_conversion.py
Checklist Results
| Check | Result | Notes |
|---|---|---|
| Inheritance correctness | N/A | No YAML schema files modified |
| Inlining of nested objects | N/A | No YAML schema files modified |
| Slot completeness (range, title, description) | N/A | No YAML schema files modified |
| Enum integrity (alphabetical, descriptions) | N/A | No YAML schema files modified |
| Generated artifacts | PASS | Regression tests added covering happy path, no-op for unknown properties, and empty-class-name guard |
Findings
Informational
-
tests/test_linkml_schema_conversion.py— Schema version bump may be warranted (coverage)
The newrestore_base_attribute_descriptionspost-processing step materially changes the content of ~50 property descriptions in generated JSON schemas across Biospecimen and Clinical. If the source YAML files carry aversion:field, a bump could help with release tracking. No action required from the author — purely a heads-up for downstream consumers.
Verdict
APPROVE
Rules defined in CLAUDE.md · To update review rules, edit CLAUDE.md and open a PR to main
ykatariy
left a comment
There was a problem hiding this comment.
Looks great!! Thank you:)
Pre-existing failure on main (surfaced by CI on this PR, unrelated to the description fix): the none-types share is 0.04%, below the test's 0.1% floor. Large external controlled vocabularies merged into main (ICD-O-3 morphology, ICD-10, antineoplastic agents) are almost entirely non-'none', so a fixed lower percentage bound is no longer valid. Keep the meaningful checks — total_none_values > 0 (none-handling exists) and the <=10% upper bound (not excessive) — and remove the lower bound.
Superseded by updated review on commit 802bd04
There was a problem hiding this comment.
HTAN Schema Review — ✅ Approved
Automated review by htan-claude · commit
802bd041842c0416b86d59fd145d20944451cc2f
Solid bug fix with a clean implementation — all schema source files are untouched, and the two informational notes from the coverage reviewer are worth surfacing for awareness.
Tip
All checklist items pass — no blocking issues found.
Files Changed
scripts/linkml_to_flat_synapse_jsonschema.pytests/test_linkml_schema_conversion.pytests/test_none_types_analysis.py
Checklist Results
| Check | Result | Notes |
|---|---|---|
| Inheritance correctness | N/A | No YAML schema changes in this PR |
| Inlining of nested objects | N/A | No YAML schema changes in this PR |
| Slot completeness (range, title, description) | N/A | No YAML schema changes in this PR |
| Enum integrity (alphabetical, descriptions) | N/A | No YAML schema changes in this PR |
| Generated artifacts | PASS | Generated files are downstream of the script fix; regression tests confirm correctness |
Findings
Informational
-
tests/test_linkml_schema_conversion.py— Missing test for no-op (already-correct description) branch (coverage)
The newTestRestoreBaseAttributeDescriptionssuite covers slot_usage override replacement, unknown properties, and empty class name, but does not exercise the branch whereprop_val["description"]already matches the canonical attribute description — meaningrestoredstays 0 and the value is left unchanged. Adding a test that asserts the description is not modified and the restored count remains 0 would complete branch coverage for the equality check. Low priority, but worth a follow-up. -
scripts/linkml_to_flat_synapse_jsonschema.py— Schema version bump may be warranted (coverage)
This fix meaningfully changes the human-readabledescriptionfield for an estimated ~50 attributes across Biospecimen and Clinical in all generated JSON schemas. If the source YAML modules carry aversion:field, bumping it would signal downstream consumers (e.g., BigQuery tabular release pipelines) that content has changed. If versioning is managed separately or the team treats this as a pure bug fix with no semantic version impact, no action is needed.
Verdict
APPROVE
Rules defined in CLAUDE.md · To update review rules, edit CLAUDE.md and open a PR to main
Fixes #182. Expand the terse 'Unique identifier for the panel' description to state the HTAN identifier format (P-prefix segment) and that it carries the same requirements as the HTAN_PANEL_ID in the corresponding ChannelMetadata RecordSet, so the Spatial and channel-metadata panel IDs are documented consistently.
Superseded by updated review on commit e9de58b
There was a problem hiding this comment.
HTAN Schema Review — ✅ Approved
Automated review by htan-claude · commit
e9de58bff3db7b4107a88fdd1fbb08034465ff0f
This is a clean, well-scoped PR — all three reviewers found nothing to flag, and the changes are exactly what you'd hope to see for a targeted bug fix.
Tip
All checklist items pass — no issues found.
Files Changed
modules/SpatialOmics/domains/spatial_panel.yamlscripts/linkml_to_flat_synapse_jsonschema.pytests/test_linkml_schema_conversion.pytests/test_none_types_analysis.py
Checklist Results
| Check | Result | Notes |
|---|---|---|
| Inheritance correctness | N/A | No inheritance chain changes introduced |
| Inlining of nested objects | N/A | No new class-range slots introduced |
| Slot completeness (range, title, description) | PASS | HTAN_PANEL_ID retains all required fields; description update is well-formed |
| Enum integrity (alphabetical, descriptions) | N/A | No enum changes in this PR |
| Generated artifacts | N/A | Generated files excluded from diff per scope rules |
Verdict
APPROVE
Rules defined in CLAUDE.md · To update review rules, edit CLAUDE.md and open a PR to main
Auto-generated Python classes from LinkML schema updates. **Auto-generated by GitHub Actions workflow** [skip ci]
|
✅ Python classes auto-updated! modules/Clinical/src/htan_clinical/datamodel/clinical.py |
Summary
Makes the generated flat Synapse JSON schemas carry the canonical attribute descriptions, and clarifies the SpatialPanel
HTAN_PANEL_IDdocumentation.Closes #191, closes #190, closes #182.
What's included
1. Generator: restore descriptions clobbered by
slot_usage(#191, #190)scripts/linkml_to_flat_synapse_jsonschema.pyemitted induced-slot descriptions, so a classslot_usagecarrying a conditional note (e.g.Required when X is "Yes") replaced the real attribute description in the output — affecting ~50 conditional attributes across Biospecimen and Clinical, which feed the BigQuery tabular release.restore_base_attribute_descriptions()runs after generation and overrides each property's description with the canonical attribute-level description (walking the class + ancestors, ignoringslot_usage).rules(if/then) constraints. Only the human-readable description changes.SECTION_THICKNESS_VALUEnow reads "…measured in microns" (Documentation descriptions #190);ACQUISITION_METHOD_OTHER_SPECIFYreads "A custom acquisition method" (Attribute Description Mismatch from LinkML to JSON Schema #191); Clinical Therapy/Diagnosis have noRequired when …descriptions left.AVERAGE_INSERT_SIZE) are unchanged; modules withoutslot_usageoverrides regenerate identically.tests/test_linkml_schema_conversion.py.2. SpatialPanel
HTAN_PANEL_IDdescription (#182)Expanded the terse "Unique identifier for the panel" to state the HTAN identifier format (P-prefix segment, e.g.
HTA201_1_P1) and that it carries the same requirements as theHTAN_PANEL_IDin the corresponding ChannelMetadata RecordSet — documenting the Spatial and channel-metadata panel IDs consistently.3. Pre-existing test fix (unrelated, unblocks CI)
test_none_types_analysisasserted the "none"-value share is 0.1%–10%; it is now 0.04% onmainbecause large controlled-vocabulary enums (ICD-O-3, ICD-10, antineoplastic agents) are almost entirely non-"none". This fails onmainindependent of this PR (which changes zero enums). Removed the invalid lower bound; kept the meaningful checks (total_none_values > 0and the ≤10% ceiling).Not in this PR
JSON_Schemas/*— produced by the downstreamjson-schema-synapseworkflow.docs/*.md— need a manualgen-docpass to reflect corrected descriptions (the LinkML source was already correct).HTAN_PANEL_IDregex for vendor panel names) — a validation/identifier change needing a design decision; tracked separately.Testing
pytest tests/→ 27 passed (3 new regression tests).