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

un-whitelist deprecation warnings #1877

Merged
merged 1 commit into from Sep 7, 2023

Conversation

hamogu
Copy link
Contributor

@hamogu hamogu commented Sep 7, 2023

We have whitelisted a number of deprecation warnings and others in our testing. I believe that some of them are not longer relevant and thus should be removed. I'm pushing this as a PR so we get CI to run and see if any of our current tests fail with this.

Some of them date back to the Python 2.7 era according to a git blame.

Why do I care? I just had tests fail locally with an "np.asscalar(a) is deprecated" and I wondered why. I looked into Sherpa and did not find the string "asscalar" anywhere (because @DougBurke probably fixed that years ago). So, why did my test fail locally? Well, I had a recent version of numpy where assaclar has actually been removed and an older version of one of our other dependencies (astropy), which still used it. Since we whitelist this warning (even after Sherpa itself has been fixed) I;d never noticed that that particular environment still had such an old version of astropy.

So, I might as well try and see how many of those warning we can remove from our whitelist and still pass our current matrix of tests.

@hamogu hamogu marked this pull request as draft September 7, 2023 16:31
@hamogu hamogu added area:tests note: easy review Indicates PR requires simple review labels Sep 7, 2023
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #1877 (f2adb28) into main (b230164) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1877   +/-   ##
=======================================
  Coverage   81.24%   81.24%           
=======================================
  Files          73       73           
  Lines       26461    26461           
  Branches     3962     3962           
=======================================
  Hits        21497    21497           
  Misses       4803     4803           
  Partials      161      161           
Files Changed Coverage Δ
sherpa/conftest.py 93.05% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b230164...f2adb28. Read the comment docs.

@DougBurke
Copy link
Contributor

Ideally I'd like the warnings explicitly hidden at the call site, as that would

a) not hide the warnings from other code
b) let us know exactly where they happen and make changes more obvious

but it's a lot of work to do (and I'm seeing some bizarre recwarn behavior that may depend on pytest version and almost-certainly on my understanding of things), so I don't see this happening any time soon.

So just removing the warnings is a subtle way to implement this!

There are two use cases that we can't check here

  • a user with a random set of modules who is trying to test their build of Sherpa
  • the CIAO test builds of Sherpa

but you and I are exemplars of the former, and I'm hoping that our CI runs are close-enough to the CIAO build systems that it shouldn't be a problem., If there are differences then it suggests that funky things are afoot that we should look at anyway....

@hamogu hamogu marked this pull request as ready for review September 7, 2023 19:29
@hamogu
Copy link
Contributor Author

hamogu commented Sep 7, 2023

Since CI passed, I'm hoping that this is good. I've asked for a CM review (@Marie-Terrell was suggested by GH first in the list before I saw @harlantc but input fomr either of you is welcome).

I suggest to put this in sooner rather than later, so that we have a little time left to reactivate any of those white listings if we see warnings and failing tests cropping up in some obscure (i.e. not CI tested) combination of version dependencies.

Copy link
Contributor

@DougBurke DougBurke left a comment

Choose a reason for hiding this comment

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

This whitelist approach is always going to be something we have to review from time to time,so thanks for doing it. I'm a lot happier nowadays as we have

a) a reasonable coverage of build setups on CI
b) more tests than we used to

Copy link
Contributor

@Marie-Terrell Marie-Terrell left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'll double check this in CIAO once it goes in

@hamogu
Copy link
Contributor Author

hamogu commented Sep 7, 2023

And to @DougBurke I made this exactly because I had a random assortment of packages (in this case numpy 1.25 and astropy 5.0) and in numpy 1.25 the function in question was actually removed because the deprecation period was over, but my astropy was so old that it still called np.asscalar. There is no way to make that right except by upgrading astropy, so if a user (e.g. me) has an environment with sherpa 4.16 and dependencies that are so old that these warnings are thrown (i.e. at least four numpy verison back), I think we want the warning to be shown to entice users to upgrade whatever dependency throws that warning.

The other whitelistings I removed where put in 7 years ago in the Python 2.7 era and we know Sherpa does not run on 2.7 any longer anyway.

@wmclaugh wmclaugh merged commit 95b5b9b into sherpa:main Sep 7, 2023
18 checks passed
@hamogu hamogu deleted the remove_deprecation_warning branch October 5, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:tests note: easy review Indicates PR requires simple review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants