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

remove deprecated models #171

Merged
merged 6 commits into from
Apr 5, 2024
Merged

remove deprecated models #171

merged 6 commits into from
Apr 5, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Jun 13, 2023 •
edited
Loading

As this is removing deprecated features this should technically trigger a 2.0 release. As I believe the removal is only to remove warnings from test suite runs I don't imagine this is urgent. However I don't see any reason not to move to 2.0.

Removed -> use this instead
DrizProductModel -> ImageModel
DataModel -> JwstDataModel
MultiProductModel -> MultiSlitModel
MIRIRampModel -> RampModel

See comment below for cross references to jwst and crds.
jwst: #171 (comment)
crds: #171 (comment)

Dropping these models required updating the minimum crds to 11.17.1:
https://github.com/spacetelescope/crds/releases/tag/11.17.1

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@codecov
Copy link

codecov bot commented Jun 13, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.81%. Comparing base (4d7c3a6) to head (cbd10e5).
Report is 23 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   64.84%   64.81%   -0.03%     
==========================================
  Files         103      102       -1     
  Lines        5694     5690       -4     
==========================================
- Hits         3692     3688       -4     
  Misses       2002     2002

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

@braingram braingram mentioned this pull request Jun 13, 2023
Merged
7 tasks
@braingram
Copy link
Collaborator Author

jwst.datamodels currently import DataModel which is causing the failure in the downstream jwst job.
https://github.com/spacetelescope/stdatamodels/actions/runs/5260129931/jobs/9506625097?pr=171#step:10:309
That failure could be fixed by: spacetelescope/jwst#7605 (which will should automatically sync the models in the two packages).

@braingram
Copy link
Collaborator Author

braingram commented Jun 14, 2023 •
edited
Loading

It appears jwst.datamodels.DataModel is used by crds:
https://github.com/spacetelescope/crds/blob/2fc85d58110885c3d3afec8c68306803856126da/crds/jwst/locate.py#L58
and there are a few uses in jwst (mostly tests and docs):
spacetelescope/jwst#7607

Removing this will require updating crds and jwst so leaving this PR as draft.

This was referenced Jun 14, 2023
Merged
Merged
@braingram
Copy link
Collaborator Author

The current jwst downstream errors I believe will be addressed in:
spacetelescope/jwst#7609

@braingram braingram marked this pull request as ready for review June 27, 2023 19:48
@braingram braingram requested a review from a team as a code owner June 27, 2023 19:48
tapastro
tapastro approved these changes Jun 27, 2023
View reviewed changes
Copy link
Collaborator

@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

Looks good to me - presumably 1.6.1 will be released shortly, and separately from 2.0.0, leading to the two unreleased headers in the changelog?

@braingram
Copy link
Collaborator Author

braingram commented Jun 27, 2023 •
edited
Loading

Looks good to me - presumably 1.6.1 will be released shortly, and separately from 2.0.0, leading to the two unreleased headers in the changelog?

Thanks for taking a look!

Yeah I wasn't quite sure the best way to handle that. The existing changes in the 1.6.1 heading presumably weren't enough to trigger a 2.0.0. I think some options are:

  1. release 1.6.1, update this PR to fix the changelog conflict, merge this PR and put all new changes in a 2.0.0 release
  2. update this PR to move all changes to 2.0.0 and merge this PR prior to the next release

Given the timing of a JWST release (which doesn't require this PR but might require the other changes in 1.6.1) I'm inclined to go for option '1' and hold off on merging this PR (I can convert it to draft if that's easiest).

@braingram braingram force-pushed the remove_deprecated branch 3 times, most recently from 7e72a52 to 14d0bb8 Compare July 10, 2023 14:06
@braingram
Copy link
Collaborator Author

Making this a draft until after the next jwst release.

@braingram braingram marked this pull request as draft July 10, 2023 15:50
@braingram braingram force-pushed the remove_deprecated branch 2 times, most recently from 5c49b94 to 37649c2 Compare July 24, 2023 19:50
@braingram braingram force-pushed the remove_deprecated branch 3 times, most recently from 83bbf67 to 3e78421 Compare August 16, 2023 15:35
@braingram braingram mentioned this pull request Sep 18, 2023
Merged
5 tasks
@braingram braingram added this to the 2.0.0 milestone Oct 20, 2023
@braingram braingram mentioned this pull request Mar 22, 2024
Merged
7 tasks
@braingram
remove deprecated models
298bffa 
Removed -> use this instead
  DrizProductModel -> ImageModel
  DataModel -> JwstDataModel
  MultiProductModel -> MultiSlitModel
  MIRIRampModel -> RampModel

bump crds minimum version
@braingram braingram mentioned this pull request Mar 25, 2024
Closed
5 tasks
@emolter
Copy link
Contributor

emolter commented Mar 25, 2024

This would resolve ticket JP-3585; please ensure this is closed when the PR is merged

@braingram
Copy link
Collaborator Author

I'd say let's start the 2.0.0 process. We have a milestone started:
https://github.com/spacetelescope/stdatamodels/milestone/13
that can be expanded when we identify more features/issues.

@braingram braingram marked this pull request as ready for review March 25, 2024 21:34
emolter
emolter approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@emolter emolter left a comment

Choose a reason for hiding this comment

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

LGTM

pyproject.toml
@@ -49,7 +49,7 @@ test = [
"psutil",
"pytest>=4.6.0",
"pytest-doctestplus",
"crds>=11.16.14",
"crds>=11.17.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

why the bump in CRDS version?

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 giving this a look.

Earlier version of crds use DataModel. See:
https://github.com/spacetelescope/crds/releases/tag/11.17.1
and:
spacetelescope/crds#938

@emolter emolter mentioned this pull request Mar 26, 2024
Merged
7 tasks
@braingram braingram merged commit 4041d5f into spacetelescope:main Apr 5, 2024
21 checks passed
@braingram braingram deleted the remove_deprecated branch April 5, 2024 18:44
@stscijgbot-jp stscijgbot-jp mentioned this pull request Apr 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tapastro tapastro tapastro approved these changes

@emolter emolter emolter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@braingram @emolter @tapastro