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.

Sign up for GitHub

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

Jump to bottom

improve schema info collection combiner handling #1875

Merged
merged 8 commits into from
Jan 9, 2025
Merged

improve schema info collection combiner handling #1875

merged 8 commits into from
Jan 9, 2025

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Nov 27, 2024 •
edited
Loading

Description

schema_info currently fails to find both subschemas and attributes of subschemas for schemas that use certain "combiners" (like allOf) and/or references.

This PR updates schema_info to support more schema configurations (including $ref and allOf usage).

Requires spacetelescope/rad#525 as the rad schemas contain invalid extra $ref items. EDIT: rad PR was merged see comment below about compatibility (tldr; we'll want to set the asdf min in rad to >4.0.x when the changes in this PR are released)

Tasks

  • run pre-commit on your machine
  • run pytest on your machine
  • Does this PR add new features and / or change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update relevant docstrings and / or docs/ page
    • for any new features, add unit tests
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: bug fix
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@braingram braingram changed the title apply hotfix for schema info collection combiner handling improve schema info collection combiner handling Nov 27, 2024
@braingram braingram force-pushed the combined_schema_info branch from f9eda46 to f1b5a8f Compare December 10, 2024 17:26
@braingram braingram mentioned this pull request Dec 12, 2024
Open
@braingram braingram marked this pull request as ready for review December 16, 2024 20:29
@braingram braingram requested a review from a team as a code owner December 16, 2024 20:29
@braingram
Copy link
Contributor Author

Specutils error comes from a bug in gwcs with astropy 7 that is fixed in the development version but not yet released.

The weldx failure is from weldx pinning asdf to <4.

One thing to consider with this PR is that the changes will (correctly) cause schema_info to not pick up archive_catalog info for old rad schemas (non-development versions, see spacetelescope/rad#525). The latest released version of rad has an asdf upper pin for <4. So as long as we merge and release this before the next rad release and add a rad pin for >= 4.0.1 (if 4.0.1 is the asdf release version) we should avoid issues.

@braingram braingram force-pushed the combined_schema_info branch from f1b5a8f to 2ad9546 Compare December 19, 2024 19:10
@braingram braingram force-pushed the combined_schema_info branch from 2ad9546 to afa9cbd Compare December 23, 2024 22:11
@braingram braingram mentioned this pull request Jan 3, 2025
Merged
6 tasks
perrygreenfield
perrygreenfield reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

Mainly asking for more helpful code documentation.

asdf/_node_info.py
return None


def _get_schema_key(schema, key):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful to add explanations of what distinguishes _get_schema_key from _get_subschema_for_property. In both cases key is an argument, in the first the key presumably is expected to be one of the properties, and in the second, key is not a property. Examples of the different usage contexts to distinguish the two might be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! How does e39d1e1 look?

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

perrygreenfield
perrygreenfield approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM

asdf/_node_info.py
return None


def _get_schema_key(schema, key):
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@braingram braingram merged commit 8c0fa2f into asdf-format:main Jan 9, 2025
46 checks passed
@braingram braingram deleted the combined_schema_info branch January 9, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perrygreenfield perrygreenfield perrygreenfield approved these changes

@nden nden Awaiting requested review from nden

Assignees
No one assigned
Labels
Downstream CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@braingram @perrygreenfield