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

switch tag keywords for external tags to wildcards #370

Merged
merged 3 commits into from
Feb 9, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Feb 8, 2024

This PR switches the tag keywords in the rad schemas to wildcard expressions for all tags outside of rad. For example:

tag: tag:stsci.edu:asdf/core/ndarray-1.0.0

is replaced by

 tag: tag:stsci.edu:asdf/core/ndarray-1.*

The use of the wildcard * means that any ndarray tag with a version starting with 1. will be considered valid.

This change is needed for asdf standard 1.6.0 (the current development version and upcoming stable version) which contains an increase in the ndarray tag version (from 1.0.0 to 1.1.0). Many other tags will also see version changes to comply with 1.6.0 (including time, quantity, etc).

The type of failure that this PR will avoid can be seen in the following example:

import roman_datamodels.maker_util
n = roman_datamodels.maker_utils.mk_node(roman_datamodels.stnode.WfiImage)
m = roman_datamodels.datamodels.ImageModel(n)
m.save('foo.asdf', version='1.6.0')  # the version is provided here as the current default is 1.5.0

which results in an error:

ValidationError: mismatched tags, wanted 'tag:stsci.edu:asdf/time/time-1.1.0', got 'tag:stsci.edu:asdf/time/time-1.2.0'

Regression tests passed: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FRoman-Developers-Pull-Requests/detail/Roman-Developers-Pull-Requests/598/pipeline/247

Checklist

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1f40287) 95.32% compared to head (21496b7) 95.32%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #370   +/-   ##
=======================================
  Coverage   95.32%   95.32%           
=======================================
  Files           4        4           
  Lines         171      171           
=======================================
  Hits          163      163           
  Misses          8        8           

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

@braingram braingram marked this pull request as ready for review February 8, 2024 17:53
Copy link
Collaborator

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

One question.

Comment on lines +50 to +55
"tag:stsci.edu:asdf/time/time-1.*",
"tag:stsci.edu:asdf/core/ndarray-1.*",
"tag:stsci.edu:asdf/unit/quantity-1.*",
"tag:stsci.edu:asdf/unit/unit-1.*",
"tag:astropy.org:astropy/units/unit-1.*",
"tag:astropy.org:astropy/table/table-1.*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I recall how this gets used, with this change the use of external tags with a 1.* version instead of a 1.0.0 (or whatever version) will be enforced by this test. Can you confirm that we get test failures if a tag a non * version external tag is used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for taking a look! Your recollection is spot-on. I made a draft PR which changes time-1.* to time-1.0 in the exposure schema (see commit). This causes the mentioned test to fail with:

E           AssertionError: assert 'tag:stsci.edu:asdf/time/time-1.0.0' in {'asdf://stsci.edu/datamodels/roman/tags/aperture-1.0.0', 'asdf://stsci.edu/datamodels/roman/tags/associations-1.0.0',...models/roman/tags/calibration_software_version-1.0.0', 'asdf://stsci.edu/datamodels/roman/tags/coordinates-1.0.0', ...}

See: https://github.com/spacetelescope/rad/actions/runs/7846209023/job/21412354776?pr=371#step:10:104

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, I wanted to make sure that future changes wouldn't slowly undo this work.

@WilliamJamieson
Copy link
Collaborator

Please do a regression test run. It should not cause any issues, but with the impending build release its better to be safe.

@braingram
Copy link
Collaborator Author

@WilliamJamieson
Copy link
Collaborator

@PaulHuwe please let us know if your happy with these changes.

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. Please confirm with @ddavis-stsci before merging (or merge after the release).

@braingram
Copy link
Collaborator Author

@ddavis-stsci I updated this PR moving the changelog entry to 0.19.1 (unreleased). As the 0.19.0 release is out any objection to merging this PR?

@ddavis-stsci
Copy link
Collaborator

Since 0.19.0 is out merging this should be fine.

@braingram
Copy link
Collaborator Author

It appears I do not have write access to this repository so please feel free to merge whenever.

@PaulHuwe PaulHuwe merged commit e85f964 into spacetelescope:main Feb 9, 2024
13 checks passed
@PaulHuwe
Copy link
Collaborator

PaulHuwe commented Feb 9, 2024

Merged.

@nden nden added this to the Build 14 milestone Feb 26, 2024
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.

6 participants