Skip to content

Reorganize Schemas to allow for better schema reviews#586

Merged
WilliamJamieson merged 11 commits into
spacetelescope:mainfrom
WilliamJamieson:full_reorg
May 5, 2025
Merged

Reorganize Schemas to allow for better schema reviews#586
WilliamJamieson merged 11 commits into
spacetelescope:mainfrom
WilliamJamieson:full_reorg

Conversation

@WilliamJamieson
Copy link
Copy Markdown
Collaborator

Closes #577

This PR Is the full version of PR #578, incorporating all current schema files.

This PR also adds unit tests which check that previously published (to PyPi designated by a tag) are not altered. These tests along with the tests on the latest schemas for keeping URIs within the latest schemas up to date should guide developers through the process of deciding if schemas need to be bumped and handling the cascade of related changes throughout RAD.

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

Comment thread tests/test_versioning.py
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.17%. Comparing base (6bedde9) to head (e0a892b).
⚠️ Report is 168 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
- Coverage   96.69%   96.17%   -0.52%     
==========================================
  Files           4        8       +4     
  Lines         272      445     +173     
==========================================
+ Hits          263      428     +165     
- Misses          9       17       +8     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@WilliamJamieson WilliamJamieson force-pushed the full_reorg branch 4 times, most recently from 271c821 to cd730a7 Compare April 25, 2025 18:20
Comment thread tests/test_versioning.py Outdated
Copy link
Copy Markdown
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I left a few comments and have a few general questions/suggestions.

The added versioning tests will be a welcome improvement. Are they (and the other test reorganization) required for this PR? If not would you split those off into a different PR(s)?

Is there any reason to include the latest files in the wheel/distribution? I can't think of one since if everything is configured as expected they are duplicates of the latest schemas/manifests. They are being included with the current PR but perhaps some pyproject.toml modification could exclude them or alternatively they could be moved outside of the src directory.

Finally, what are your thoughts on exluding the fps and tvac files from latest? It's my understanding these should never change.

Comment thread tests/conftest.py Outdated
Comment thread docs/versioning.rst Outdated
Comment thread docs/versioning.rst Outdated
Comment thread tests/conftest.py Outdated
Comment thread tests/conftest.py Outdated
@WilliamJamieson
Copy link
Copy Markdown
Collaborator Author

WilliamJamieson commented Apr 28, 2025

The added versioning tests will be a welcome improvement. Are they (and the other test reorganization) required for this PR? If not would you split those off into a different PR(s)?

The "latest" tests are by definition part of this PR as they are literally testing that the files in the reorganization follow the scheme we intend them to follow.

Technically the versioning tests can be separated and merged before these changes, though I based the tests off the changes in made for the latest tests which means it will be annoying to sort the history out. In either case, I think the versioning tests are rather important to keep us consistent with the schema versioning principles.

Is there any reason to include the latest files in the wheel/distribution? I can't think of one since if everything is configured as expected they are duplicates of the latest schemas/manifests. They are being included with the current PR but perhaps some pyproject.toml modification could exclude them or alternatively they could be moved outside of the src directory.

I don't know why they are being included, the pyproject.toml file inclusion (which I did not touch) seems to indicate to me that they should not be included. Honestly, I don't feel that it matters that much as they amount to ~700KB and should not grow that much in size. If you feel that is essential I will attempt to figure out how to exclude them.

Finally, what are your thoughts on exluding the fps and tvac files from latest? It's my understanding these should never change.

The latest files do NOT indicate the files that can be changed. Indeed, as it stands right now, no current schema file in RAD should be changed without a version change (aside from changes mentioned in the docs). Instead, the latest simply indicates the latest version of a given schema. Thus I think it is fine to leave them as they are.

@WilliamJamieson WilliamJamieson force-pushed the full_reorg branch 2 times, most recently from 5152f5d to d76e9c9 Compare April 28, 2025 19:16
@PaulHuwe
Copy link
Copy Markdown
Collaborator

Finally, what are your thoughts on exluding the fps and tvac files from latest? It's my understanding these should never change.

RTB has discussed possibly updating these schemas, so it is probably good to include them in this versioned remodel.

Copy link
Copy Markdown
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.

Some small suggestions, but otherwise, it looks great. I am eager to see it installed.

Comment thread docs/versioning.rst Outdated
Comment thread tests/test_latest.py Outdated
Comment thread tests/test_latest.py Outdated
Comment thread tests/test_latest.py Outdated
Comment thread tests/test_versioning.py Outdated
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

@WilliamJamieson is it no longer the plan to pull the non-latest test reorganization out into a separate PR(s)?

@WilliamJamieson
Copy link
Copy Markdown
Collaborator Author

@WilliamJamieson is it no longer the plan to pull the non-latest test reorganization out into a separate PR(s)?

@PaulHuwe what is your opinion?

Comment thread MANIFEST.in Outdated
@WilliamJamieson
Copy link
Copy Markdown
Collaborator Author

@WilliamJamieson is it no longer the plan to pull the non-latest test reorganization out into a separate PR(s)?

@braingram see #592. The changes here have been rebased on top of that PR.

@braingram
Copy link
Copy Markdown
Collaborator

@braingram see #592. The changes here have been rebased on top of that PR.

Thanks. I will take a look at that PR.

@WilliamJamieson
Copy link
Copy Markdown
Collaborator Author

@braingram see #592. The changes here have been rebased on top of that PR.

Thanks. I will take a look at that PR.

To recap the RAD discussion since it sounds easier/required to have #592 before this PR and we would benefit from having them both sooner rather than later I'm ok having:

  • the "latest" schema updates
  • the test reorganization
  • the "versioning" tests

all be part of this PR (and closing #592). Does that work for you @WilliamJamieson? If so I'll focus on reviewing this PR instead of #592.

This works for me. I'll close #592.

@WilliamJamieson WilliamJamieson force-pushed the full_reorg branch 6 times, most recently from 4db69b4 to e1adcc6 Compare May 2, 2025 15:34
@WilliamJamieson WilliamJamieson mentioned this pull request May 2, 2025
8 tasks
Copy link
Copy Markdown
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Thanks again for putting this together. I left a couple comments/suggestions. The flattening during across version comparisons is losing some items. I left a comment/suggestion for fixing this and using an asdf method.

Comment thread tests/test_versioning.py Outdated
Comment thread tests/test_versioning.py Outdated
Comment thread tox.ini
Comment thread tests/test_versioning.py Outdated
WilliamJamieson and others added 11 commits May 5, 2025 16:38
Co-authored-by: Brett Graham <brettgraham@gmail.com>
Apply suggestions from code review

Co-authored-by: Brett Graham <brettgraham@gmail.com>
Instead only compair the current state to all the former versions.
Co-authored-by: Brett Graham <brettgraham@gmail.com>
This way the local tags will be up to date
@WilliamJamieson WilliamJamieson merged commit ef5d304 into spacetelescope:main May 5, 2025
14 of 15 checks passed
@WilliamJamieson WilliamJamieson deleted the full_reorg branch May 5, 2025 20:42
@WilliamJamieson
Copy link
Copy Markdown
Collaborator Author

For completeness, the regression tests pass just fine immediately post merge: https://github.com/spacetelescope/RegressionTests/actions/runs/14845873819/job/41679620194

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.

Schema workflow improvements

3 participants