Skip to content

Conversation

@mcara
Copy link
Member

@mcara mcara commented Oct 28, 2025

In preparation for release 3.0 of drizzle package, this PR disables treatment of deprecation warnings to be introduced in the upcoming release of the drizzle as errors during testing.

Tasks

  • If you have a specific reviewer in mind, tag them.
  • add a build milestone, i.e. Build 12.0 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see changelog readme for instructions)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly

@mcara mcara added this to the Build 12.1.1 milestone Oct 28, 2025
@mcara mcara requested review from pllim and tapastro October 28, 2025 06:19
@mcara mcara self-assigned this Oct 28, 2025
@mcara mcara requested a review from a team as a code owner October 28, 2025 06:19
@codecov
Copy link

codecov bot commented Oct 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.47%. Comparing base (38c180a) to head (e480163).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9963      +/-   ##
==========================================
+ Coverage   85.45%   85.47%   +0.01%     
==========================================
  Files         366      366              
  Lines       37845    37890      +45     
==========================================
+ Hits        32341    32386      +45     
  Misses       5504     5504              

☔ 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.

@emolter
Copy link
Collaborator

emolter commented Oct 28, 2025

shouldn't we instead update our code to use the more up-to-date syntax for drizzle calls, instead of ignoring the warnings?

@pllim
Copy link
Collaborator

pllim commented Oct 28, 2025

@emolter eventually. This is to future proof for unmerged PR upstream (spacetelescope/drizzle#203). Once it is in, we can handle the code appropriately using version check. Though to do that, might have to unpin:

"drizzle>=2.1.1,<2.2.0",

Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Thanks! Will have to open follow-up issue to un-ignore when we can, but I think this is fine for now.

@emolter
Copy link
Collaborator

emolter commented Oct 28, 2025

sorry @pllim I'm still confused. If drizzle is pinned already, then we won't encounter these DeprecationWarnings until we change the pin, right? So why do we need to ignore the warnings?

@mcara
Copy link
Member Author

mcara commented Oct 28, 2025

  1. When testing Rescale resample kernels by pixel scale ratio - V2 stcal#418 one can see that downstream tests (true, it's romancal right now) fail because of deprecation warnings being converted into errors.
  2. @braingram was concerned about backwards compatibility of the upcoming release with the already released pipeline. (Regression) Testing jwst pipeline against my drizzle PR (without the appropriate changes to stcal) would fail because of warnings->errors.

To be clear: this PR disables conversion of 4 specific deprecation warnings that will be issued by the upcoming drizzle into errors for testing purposes only. It will not hide these warnings and it will not affect normal use of the pipeline.

@pllim
Copy link
Collaborator

pllim commented Oct 28, 2025

Ned, indeed we do not need to ignore the warnings with upper pin, but we will need them or handle the version check when pin is removed. @tapastro , when does one remove upper pin for drizzle?

@tapastro
Copy link
Contributor

I may have to defer to those with more package maintenance experience - I think in an ideal world we would not have upper pins on main but would add them on all release branches?

It looks like drizzle has an upper pin now because of a flurry of PRs dealing with a yanked 2.1.0 release. Perhaps instead of the deprecation warning catching, we make a PR that removes the upper pin and addresses the deprecations directly?

@mcara
Copy link
Member Author

mcara commented Oct 28, 2025

Removing upper pin is something that can be done although we can run regression tests right now and force a specific version of drizzle to be used regardless of this pin, I believe. The issue is that regression tests will fail because of warnings being converted into errors.

I want to merge my drizzle PR but it is difficult to get more approvals. Regression test https://github.com/spacetelescope/RegressionTests/actions/runs/18597040806/job/53449590541 is failing because of warnings. So, this PR is an easier, IMO, alternative to a different proposed solution that does not require making two releases of the drizzle package with the first release introducing new parameters but not issuing deprecation warnings.

This PR does not affect anything except treatment of 4 specific warnings in pytest. I do not see what is the drawback to this.

@pllim
Copy link
Collaborator

pllim commented Oct 28, 2025

Turns out the dev requirements does not include dev version of drizzle, plus there is an upper pin, so we actually won't see these warnings until drizzle is released and we remove the upper pin. I think what Tyler proposed is as follows:

  1. Close this PR without merge. Or turn this into draft if you want to prove that this drizzle change didn't break jwst.
  2. Wait for drizzle to release with these changes.
  3. Open a new PR here to do the following:
    a. Remove upper pin.
    b. Introduce drizzle version checks and handle the deprecations appropriately without ignoring warnings.
    c. (Optional) Add drizzle to requirements-dev-st.txt for future-proofing in devdeps.

@mcara
Copy link
Member Author

mcara commented Oct 28, 2025

Turns out the dev requirements does not include dev version of drizzle, plus there is an upper pin, so we actually won't see these warnings until drizzle is released and we remove the upper pin. I think what Tyler proposed is as follows:

  1. Close this PR without merge. Or turn this into draft if you want to prove that this drizzle change didn't break jwst.
  2. Wait for drizzle to release with these changes.
  3. Open a new PR here to do the following:
    a. Remove upper pin.
    b. Introduce drizzle version checks and handle the deprecations appropriately without ignoring warnings.
    c. (Optional) Add drizzle to requirements-dev-st.txt for future-proofing in devdeps.

I am not sure how this will address testing issues. First of all, there are no changes that need to be done in jwst (besides removing upper pin if that is what you want but in regression test I can override this and so this would affect only next release of jwst). The code that deals with deprecations is in stcal.

@mcara
Copy link
Member Author

mcara commented Oct 28, 2025

Also,

Wait for drizzle to release with these changes.

The point is to test drizzle with existing jwst (stcal) code to see that it does not break anything before making a release.

@tapastro
Copy link
Contributor

If the only failures seen are deprecation warnings elevated to errors, then I think the mission is accomplished?

Alternatively, you could run a set of regression tests and do not capture the deprecation warnings - any errors would remain. I tried to start that run here: https://github.com/spacetelescope/RegressionTests/actions/runs/18883720965

@melanieclarke
Copy link
Collaborator

@mcara - is this PR still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants