Remove `dqflags` and related code. #146

WilliamJamieson · 2023-03-01T20:49:32Z

This PR removes the code moved to stdatamodels in spacetelescope/stdatamodels#134. I don't think any of the dqflags related code is actually used outside the jwst pipeline, so this should not effect any packages.

Note, I replaced the removed code with thin wrapper modules which raise a deprecation warning and then attempt to import from stdatamodels, raising a useful error if that import fails.

Closes #145

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 · 2023-03-01T22:29:48Z

Codecov Report

Patch coverage: 88.57% and project coverage change: +0.18 🎉

Comparison is base (6eff679) 72.13% compared to head (245fff9) 72.32%.

❗ Current head 245fff9 differs from pull request most recent head 038bd07. Consider uploading reports for the commit 038bd07 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   72.13%   72.32%   +0.18%     
==========================================
  Files          28       29       +1     
  Lines        5470     5459      -11     
==========================================
+ Hits         3946     3948       +2     
+ Misses       1524     1511      -13

Flag	Coverage Δ
unit	`72.32% <88.57%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/stcal/basic_utils.py	`85.71% <85.71%> (-14.29%)`	⬇️
src/stcal/dqflags.py	`85.71% <85.71%> (-6.60%)`	⬇️
src/stcal/dynamicdq.py	`85.71% <85.71%> (+58.44%)`	⬆️
tests/test_dq.py	`92.30% <92.30%> (-7.70%)`	⬇️
src/stcal/conftest.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

WilliamJamieson · 2023-03-01T22:56:08Z

Converting to a draft because spacetelescope/stdatamodels#134 needs to be merged and released before these changes will fully work.

PaulHuwe

Should more dependencies be added to tox.ini? When I run STCAL tests, I get the following errors:

FAILED tests/test_dq.py::test_deprecation[dqflags] - ImportError: dqflags has been moved to stdatamodels.dqflags, please install stdatamodels
FAILED tests/test_dq.py::test_deprecation[dynamicdq] - ImportError: dynamicdq has been moved to stdatamodels.dynamicdq, please install stdatamodels
FAILED tests/test_dq.py::test_deprecation[basic_utils] - ImportError: basic_utils has been moved to stdatamodels.basic_utils, please install stdatamodels

WilliamJamieson · 2023-03-08T19:52:41Z

Should more dependencies be added to tox.ini? When I run STCAL tests, I get the following errors:

FAILED tests/test_dq.py::test_deprecation[dqflags] - ImportError: dqflags has been moved to stdatamodels.dqflags, please install stdatamodels FAILED tests/test_dq.py::test_deprecation[dynamicdq] - ImportError: dynamicdq has been moved to stdatamodels.dynamicdq, please install stdatamodels FAILED tests/test_dq.py::test_deprecation[basic_utils] - ImportError: basic_utils has been moved to stdatamodels.basic_utils, please install stdatamodels

I don't think you have the right things installed, you need to try re-installing.

PaulHuwe · 2023-03-08T20:12:51Z

Details

Turns out that having too old a version of stdatamodels installed caused this.

WilliamJamieson · 2023-03-08T20:14:25Z

Details

Turns out that having too old a version of stdatamodels installed caused this.

If stdatamodels is not installed at all, no error will be raised.

PaulHuwe · 2023-03-08T22:32:27Z

Details

After some further investigation, I figured it out. With an unsupported version of stdatamodels installed, tox would work, but pytest would fail with what I pasted above. Romancal installs stdatamodels via the stpipe dependency.

When I installed the development version of stdatamodels, things worked fine.

PaulHuwe

With dev stdatamodels, this passes all romancal unit and regression tests.

hbushouse

Do we need a new release of stdatamodels before a new release of stcal?

WilliamJamieson · 2023-03-09T19:17:21Z

Do we need a new release of stdatamodels before a new release of stcal?

Yes

This is being moved to stdatamodels

`dynamicdq` is being moved to stdatamodels

hbushouse · 2023-03-14T14:44:32Z

Is there a way to get the 3 queued CI tests to start?

WilliamJamieson · 2023-03-14T14:48:58Z

Is there a way to get the 3 queued CI tests to start?

Short answer is no, the spacetelescope organization has a very limited number of macos runners, so if lots of PRs in spacetelescope are made, those can get really backed up.

I'm not sure why we have so many macos jobs across all our CI. In general you only really need one to make sure we aren't making assumptions that macos cannot handle, not one on every single version of python.

@zacharyburnett can provide more information.

zacharyburnett · 2023-03-14T14:51:20Z

Is there a way to get the 3 queued CI tests to start?

Short answer is no, the spacetelescope organization has a very limited number of macos runners, so if lots of PRs in spacetelescope are made, those can get really backed up.

I'm not sure why we have so many macos jobs across all our CI. In general you only really need one to make sure we aren't making assumptions that macos cannot handle, not one on every single version of python.

@zacharyburnett can provide more information.

That's a fair point; I added macos jobs for thoroughness but you're right, we have limited resources available for free. If you guys are okay with it, I can make some PRs removing macos jobs for Python versions less than latest across the organization.

WilliamJamieson · 2023-03-14T14:57:58Z

That's a fair point; I added macos jobs for thoroughness but you're right, we have limited resources available for free.

"free" is a strong statement, I don't know exactly how STScI has structured the org, but in general an organization only gets so many minutes of action time per month before they start having to pay for it, see https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions for more details.
Note also that macOS actions have a 10x multiplier on their run time to get how many minutes they cost.

hbushouse · 2023-03-14T15:03:48Z

Well, at this point I'm satisfied that this works OK and hence am willing to merge without waiting for those remaining CI tests to complete. @WilliamJamieson you're hereby authorized to merge.

WilliamJamieson · 2023-03-14T15:07:30Z

Well, at this point I'm satisfied that this works OK and hence am willing to merge without waiting for those remaining CI tests to complete. @WilliamJamieson you're hereby authorized to merge.

I would love to but I don't have the rights in stcal to merge things if the branch protections have not passed. So I'll just set the automerge, and it will merge when the protections pass.

@kmacdonald-stsci might be able to override the protections.

hbushouse · 2023-03-14T15:08:29Z

Well, at this point I'm satisfied that this works OK and hence am willing to merge without waiting for those remaining CI tests to complete. @WilliamJamieson you're hereby authorized to merge.

I would love to but I don't have the rights in stcal to merge things if the branch protections have not passed. So I'll just set the automerge, and it will merge when the protections pass.

@kmacdonald-stsci might be able to override the protections.

I have the power. Merging now.

WilliamJamieson requested review from nden, stscieisenhamer and hbushouse March 1, 2023 20:49

github-actions bot added testing ramp_fitting labels Mar 1, 2023

WilliamJamieson marked this pull request as draft March 1, 2023 22:56

This was referenced Mar 1, 2023

Switch to using dqflags originating from stdatamodels spacetelescope/jwst#7475

Merged

JP-3075: Updating dynamic_mask function to include inversion functionality #145

Closed

WilliamJamieson requested a review from kmacdonald-stsci March 3, 2023 17:45

WilliamJamieson force-pushed the refactor/move_dq_to_stdatamodels branch from e8f9a03 to 32b6dbd Compare March 3, 2023 17:45

github-actions bot removed the ramp_fitting label Mar 3, 2023

kmacdonald-stsci approved these changes Mar 3, 2023

View reviewed changes

WilliamJamieson force-pushed the refactor/move_dq_to_stdatamodels branch from 32b6dbd to 0efbc77 Compare March 3, 2023 18:07

WilliamJamieson requested a review from PaulHuwe March 7, 2023 16:18

WilliamJamieson marked this pull request as ready for review March 8, 2023 00:29

kmacdonald-stsci closed this Mar 8, 2023

WilliamJamieson reopened this Mar 8, 2023

PaulHuwe requested changes Mar 8, 2023

View reviewed changes

PaulHuwe approved these changes Mar 8, 2023

View reviewed changes

hbushouse added this to the 1.4.0 milestone Mar 9, 2023

hbushouse added the jwst dq label Mar 9, 2023

hbushouse approved these changes Mar 9, 2023

View reviewed changes

Remove dqflags and supporting code.

c12acf9

This is being moved to stdatamodels

WilliamJamieson added 4 commits March 14, 2023 09:52

Remove dynamicdq

91af17b

`dynamicdq` is being moved to stdatamodels

Add import pass through

93fb2d7

Update changes

1132abc

Fix test collection error

038bd07

WilliamJamieson force-pushed the refactor/move_dq_to_stdatamodels branch from 245fff9 to 038bd07 Compare March 14, 2023 13:53

hbushouse approved these changes Mar 14, 2023

View reviewed changes

WilliamJamieson enabled auto-merge (squash) March 14, 2023 15:07

hbushouse disabled auto-merge March 14, 2023 15:08

hbushouse merged commit 4e6df94 into spacetelescope:main Mar 14, 2023

WilliamJamieson deleted the refactor/move_dq_to_stdatamodels branch March 14, 2023 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `dqflags` and related code. #146

Remove `dqflags` and related code. #146

WilliamJamieson commented Mar 1, 2023 •

edited by hbushouse

Loading

codecov bot commented Mar 1, 2023 •

edited

Loading

WilliamJamieson commented Mar 1, 2023

PaulHuwe left a comment

WilliamJamieson commented Mar 8, 2023

PaulHuwe commented Mar 8, 2023

WilliamJamieson commented Mar 8, 2023

PaulHuwe commented Mar 8, 2023

PaulHuwe left a comment

hbushouse left a comment

WilliamJamieson commented Mar 9, 2023

hbushouse commented Mar 14, 2023

WilliamJamieson commented Mar 14, 2023

zacharyburnett commented Mar 14, 2023

WilliamJamieson commented Mar 14, 2023

hbushouse commented Mar 14, 2023

WilliamJamieson commented Mar 14, 2023

hbushouse commented Mar 14, 2023

Remove dqflags and related code. #146

Remove dqflags and related code. #146

Conversation

WilliamJamieson commented Mar 1, 2023 • edited by hbushouse Loading

codecov bot commented Mar 1, 2023 • edited Loading

Codecov Report

WilliamJamieson commented Mar 1, 2023

PaulHuwe left a comment

Choose a reason for hiding this comment

WilliamJamieson commented Mar 8, 2023

PaulHuwe commented Mar 8, 2023

WilliamJamieson commented Mar 8, 2023

PaulHuwe commented Mar 8, 2023

PaulHuwe left a comment

Choose a reason for hiding this comment

hbushouse left a comment

Choose a reason for hiding this comment

WilliamJamieson commented Mar 9, 2023

hbushouse commented Mar 14, 2023

WilliamJamieson commented Mar 14, 2023

zacharyburnett commented Mar 14, 2023

WilliamJamieson commented Mar 14, 2023

hbushouse commented Mar 14, 2023

WilliamJamieson commented Mar 14, 2023

hbushouse commented Mar 14, 2023

Remove `dqflags` and related code. #146

Remove `dqflags` and related code. #146

WilliamJamieson commented Mar 1, 2023 •

edited by hbushouse

Loading

codecov bot commented Mar 1, 2023 •

edited

Loading