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

updates for upcoming ASDF standard 1.6.0 #324

Merged
merged 10 commits into from
Apr 15, 2024
Merged

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Feb 2, 2024

asdf is working on making the current development version of the ASDF standard (1.6.0) stable (see asdf-format/asdf#1744 for details). As the 1.6.0 version contains new versions of a few core schemas (including ndarray) many dependent schemas will require updates to be compatible with the new version (if they contain $ref or tag links to core schemas).

This PR updates the dkist schemas to be compatible with ASDF standard 1.6.0. One example of a required update (for ASDF 1.6.0 compatibility) is updating the transform $refs found in several dkist schemas to transform-1.3.0. The schema updates were accompanied by creation of a new dkist-wcs-1.3.0 extension.

Similar updates will be needed for sunpy (see: sunpy/sunpy#7432) to allow dkist to be fully compatible with ASDF standard 1.6.0. (see this run where asdf switched the default to 1.6.0 and dkist tests were run with this PR and the sunpy PR: https://github.com/asdf-format/asdf/actions/runs/8345268069/job/22843626280?pr=1744).

A few minor schema fixes were also added:

  • fix manifest ids to match uris used to register manifests with asdf
  • use a unique extension_uri for each extension
  • fix tag patterns in varying_celestial_transform schema

Several dependency changes are also included (more on this below):

  • remove asdf-unit-schemas as a dependency (this package only provided redundant schemas and has been decommissioned: https://github.com/asdf-format/asdf-unit-schemas)
  • update the minimum required asdf to 2.14.4. This was needed for 2 reasons:
    • the pytest-asdf plugin to test the examples in the new schemas (which use asdf-standard requirements)
    • old versions of asdf included experimental support for the development standard (1.6.0). However versions before 2.14 fail to fully support the current state of the ASDF 1.6.0 standard (they will write out files using the old ndarray-1.0.0 tag instead of the required ndarray-1.1.0 tag). If a user forced asdf to use the development standard they will have produced files that may not be compatible with the current ASDF 1.6.0 standard. This also means that if the minimum-dependencies CI attempts to use a version of asdf < 2.14 that it will produce invalid files and errors.
  • increase the minimum required asdf-transform-schemas version to allow the dkist schema tests in the minimum-dependencies job to find the new transform-1.3.0 schema (also increase the asdf-standard version to match the minimum in that version of asdf-transform-schemas)
  • increase the minimum required asdf-astropy version to make it aware of the updated asdf-transform-schemas to allow it to test the new schemas during the minimum-dependencies job (and increase the asdf-coordinate-schemas version to match the requirement in asdf-astropy).

Many of the minimum version bumps above were required to allow the schema tests to pass in the minimum-dependencies job. If these version increases are not an issue I think they are overall improvements (and reduce the risk that say an old environment is updated and does not pick up the required schemas). If the minimum version increases are an issue one thing to consider would be disabling the schema tests during the minimum-dependencies job.

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.74%. Comparing base (c6dd4c2) to head (a271a6e).

❗ Current head a271a6e differs from pull request most recent head 14a41b2. Consider uploading reports for the commit 14a41b2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
+ Coverage   96.36%   97.74%   +1.38%     
==========================================
  Files          51       34      -17     
  Lines        2832     1994     -838     
==========================================
- Hits         2729     1949     -780     
+ Misses        103       45      -58     

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

@braingram braingram changed the title fix manifest ids and extension_uri work-in-progress: schema cleanup Feb 2, 2024
@braingram braingram changed the title work-in-progress: schema cleanup TEST: update schemas for asdf-standard 1.6.0 Feb 8, 2024
@braingram braingram force-pushed the schema_cleanup branch 2 times, most recently from 73f86aa to 913a9c3 Compare March 19, 2024 13:35
@braingram braingram changed the title TEST: update schemas for asdf-standard 1.6.0 WIP: update schemas for asdf-standard 1.6.0 Mar 19, 2024
@braingram braingram changed the title WIP: update schemas for asdf-standard 1.6.0 updates for upcoming ASDF standard 1.6.0 Mar 19, 2024
@braingram braingram marked this pull request as ready for review March 20, 2024 18:43
@braingram braingram closed this Mar 26, 2024
@braingram braingram reopened this Mar 26, 2024
@Cadair
Copy link
Member

Cadair commented Mar 26, 2024

Hi @braingram Thanks so much for this PR (and the sunpy one). The code changes look good, but I have a couple of questions mainly so I understand this properly.

I generally try and stick to SPEC-0 dependency rules (so 2 years for most deps), but I am willing to not do this for ASDF things where it makes sense, because that seems to be a challenge.

With regards to the minimum dependencies, if we have those new minimums installed would we write files which use 1.6.0? or would it be the minimum needed to read a file written with 1.6.0?

@braingram
Copy link
Contributor Author

Hi @braingram Thanks so much for this PR (and the sunpy one). The code changes look good, but I have a couple of questions mainly so I understand this properly.

I generally try and stick to SPEC-0 dependency rules (so 2 years for most deps), but I am willing to not do this for ASDF things where it makes sense, because that seems to be a challenge.

Thanks! SPEC-0 is new to me.

asdf does not currently support older releases and unfortunately we do not have the bandwidth of the core projects mentioned in SPEC-0. To make sure I'm understanding, you'd like the minimum versions to be the latest version as of 2 years ago?

This might be possible but I suspect that this PR will instead need to restrict dkist to using ASDF standard 1.5.0 as asdf 2.11 produces invalid files if told to write out 1.6.0.

With regards to the minimum dependencies, if we have those new minimums installed would we write files which use 1.6.0? or would it be the minimum needed to read a file written with 1.6.0?

Currently 1.6.0 is not yet the default in asdf (switching that will occur at the next major version change, 4.0, which we are hoping to release in the next 6 months but don't have a hard ETA). So a call to AsdfFile.write_to with no version argument will write a 1.5.0 file.

@Cadair
Copy link
Member

Cadair commented Mar 27, 2024

To make sure I'm understanding, you'd like the minimum versions to be the latest version as of 2 years ago?

That's the default position. As I say though I am happy for there to be well reasoned exceptions.

This might be possible but I suspect that this PR will instead need to restrict dkist to using ASDF standard 1.5.0 as asdf 2.11 produces invalid files if told to write out 1.6.0.

Under what conditions would asdf 2.11 write out a 1.6.0 file?

Currently 1.6.0 is not yet the default in asdf (switching that will occur at the next major version change, 4.0, which we are hoping to release in the next 6 months but don't have a hard ETA). So a call to AsdfFile.write_to with no version argument will write a 1.5.0 file.

So asdf wont write a 1.6.0 file by default until we write a file out with asdf 4.0+ installed?

@Cadair
Copy link
Member

Cadair commented Mar 27, 2024

Oh the other thing I should make clear is that we write out files with the minimum versions of all these packages at the data center, so that we don't throw warnings about the users env being too old when it is within the minimums for the given version of the dkist package.

So I am also trying to understand what the impact would be on us starting to generate our production files with these new minimums.

@braingram
Copy link
Contributor Author

braingram commented Mar 27, 2024

Thanks for the reply. I certainly want to find a solution that fixes the most issues with minimal impact.

Under what conditions would asdf 2.11 write out a 1.6.0 file?

In asdf 2.11 it is possible to provide 1.6.0 to AsdfFile.write_to to write out a file using ASDF standard 1.6.0. It seems very unlikely that a user would do this. However asdf 2.11 does not warn or in any other way inform the user that 1.6.0 is a "development" version. If the user provided 1.6.0 asdf 2.11 would write out the file using the invalid ndarray-1.0.0 tag (if the tree contained an array) and would in most circumstances produce no warnings or errors. Attempts to read in this file with a newer version of asdf (>=2.14) would produce warnings about unknown tags and read in the arrays as tagged dictionaries.

I am not aware of any code in the asdf downstream that provides a version to AsdfFile.write_to so I think the above concern is minor (but real).

So asdf wont write a 1.6.0 file by default until we write a file out with asdf 4.0+ installed?

Exactly!

I think the main issue here is the schema tests in oldestdeps. The schema tests may force a particular ASDF standard version (see this comment on the sunpy PR) when an example uses tags that are only available in a certain ASDF standard version. This is required by many of the new schemas added in this PR (as they use new tags that are only available in ASDF standard 1.6.0). When the pytest asdf plugin sees the asdf-standard-1.6.0 string it will read in the example as ASDF standard 1.6.0. This causes issues with old asdf versions because:

  • some don't fully support the asdf-standard-1.6.0 string
  • older versions of asdf-standard don't have the same 1.6.0 (so tag mismatches will occur)
  • older versions of asdf-astropy and other schema providing packages may not have the required schemas

It's possible to disable these schema tests for oldestdeps by providing -o asdf_schema_tests_enabled=false to pytest/tox. Would you like me to update the PR removing the lower pin increases and disabling the schema tests for oldestdeps? Given that the schema tests do very little and will still be run in the other CI jobs this seems like little to no risk and should allow the lower pins to stay the same (but does not address the issue with asdf <2.14 and ASDF standard 1.6.0). EDIT: I updated the PR to use this approach, skipping the schema tests in oldestdeps

Comment on lines +63 to +68
{env:PYTEST_COMMAND} \
online: --remote-data=any \
# It's not possible to test the new schemas with the oldest dependencies
# as the new schemas require new dependent schemas
oldestdeps: -o asdf_schema_tests_enabled=false
{posargs}
Copy link
Member

Choose a reason for hiding this comment

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

oh hey, I didn't know you could do that 🤔

@Cadair Cadair enabled auto-merge (squash) April 15, 2024 10:12
@Cadair
Copy link
Member

Cadair commented Apr 15, 2024

Thanks a lot for this @braingram and taking the time to explain it all to me.

@Cadair Cadair merged commit 3b8f260 into DKISTDC:main Apr 15, 2024
15 of 18 checks passed
@braingram braingram deleted the schema_cleanup branch April 15, 2024 14:21
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.

2 participants