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

reorder anyOf in apcorr schema to work around asdf bug #542

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Feb 12, 2025

The asdf datatype validator raises an exception instead of yielding leading to incorrect validation failure for the anyOf schema combiner in the apcorr ref file schema (xref: asdf-format/asdf#1903).

This PR works around the issue (until an asdf fix can be made and released) by reordering the anyOf to check for None first (when this succeeds the remainder of the anyOf will not be evaluated avoiding the asdf bug).

Tasks

  • Update or add relevant rad tests.
  • Update relevant docstrings and / or docs/ page.
  • Does this PR change any schema files?
    • Schema changes were discussed at RAD Review Board meeting.
  • Does this PR change any API used downstream? (If not, label with no-changelog-entry-needed.)
News fragment change types:
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.61%. Comparing base (6094d86) to head (32af770).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #542      +/-   ##
==========================================
+ Coverage   96.59%   96.61%   +0.02%     
==========================================
  Files           4        4              
  Lines         264      266       +2     
==========================================
+ Hits          255      257       +2     
  Misses          9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PaulHuwe
Copy link
Collaborator

Do we need to follow this pattern for future anyOfs containing None like this?

Copy link
Collaborator

@PaulHuwe PaulHuwe left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram
Copy link
Collaborator Author

Do we need to follow this pattern for future anyOfs containing None like this?

I would say no. When asdf-format/asdf#1904 is merged asdf will handle the current schema on rad main without issue. The schema reordering avoids the bug (and otherwise functions the same way).

I do think it's worth adding the test in spacetelescope/roman_datamodels#460 since we want to be sure None values are accepted. I'll open that PR once this is merged.

@braingram braingram merged commit 244328c into spacetelescope:main Feb 12, 2025
12 of 13 checks passed
@braingram braingram deleted the apcorr_fix branch February 12, 2025 19:02
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