ENH: replace `try/except ImportError` blocks with `if HAS_X` by neutrinoceros · Pull Request #16353 · astropy/astropy

neutrinoceros · 2024-04-29T07:32:06Z

Description

Motivation: #16351 showed that we have hidden optional dependencies in a couple places, showing as modules that we attempt to import in a more-or-less recoverable fashion with plain try/except blocks such as

try:
    import x
except ImportError:
    ...
else:
    ...

but that may not all be declared as optional dependencies in package metadata.

In this PR, I'm going for the bulk of these blocks (focusing on packages that are already declared as optional deps) and replacing them with our usual idiom

if HAS_X:
    ...
else:
    ...

It some places, it leads to a user-visible improvement with how exceptions are chained, but the main idea is to make problematic occurrences of except ImportError like the one causing #16351 easier to spot.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

Approvals by sub-package

github-actions · 2024-04-29T07:32:32Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

astrofrog · 2024-04-29T08:25:09Z

astropy/io/fits/util.py

    so that non-dask code is not slowed down.
    """
-    if not hasattr(data, "compute"):
+    if not HAS_DASK or not hasattr(data, "compute"):


See above comment as choice was deliberate

Or does this PR avoid importing the optional deps to define HAS_?

Not this PR, but I did exactly that a couple months back in #15771

Ok then the comment above this can probably be removed/updated - thanks!

taldcroft

Good for io.ascii, table, time.

larrybradley

LGTM for convolution, stats, and visualization.

pllim · 2024-04-29T15:21:35Z

astropy/coordinates/solar_system.py

-    except ImportError:
-        raise ImportError(
+    if not HAS_JPLEPHEM:
+        raise ModuleNotFoundError(


Can we maybe keep the error type the same?

ModuleNotFoundError is a subclass of ImportError, so this change is backwards compatible.

eerovaher

I can approve for coordinates.

pllim

In the places where ImportError is now ModuleNotFoundError, do they need a change log? Even if the new error is backward compatible, it is still a change, right?

Also in stats, looks like ModuleNotFoundError is newly introduced (or at least has a different message now). (But from the diff, not sure what it was before since it just assumed scipy was available.)

mhvk

Thanks, that is a very nice cleanup. I feel the use of ModuleNotFound is an improvement too - and even suggest we use that in place of a ValueError in one case.

But good to go as is!

astropy/utils/xml/writer.py

neutrinoceros · 2024-04-29T17:44:49Z

In the places where ImportError is now ModuleNotFoundError, do they need a change log? Even if the new error is backward compatible, it is still a change, right?

In my opinion it's borderline not user-visible (in the sense that I doubt anyone would notice) so I don't think it's worth a mention in the changelog, but obviously I'm happy to add one still if you guys think otherwise !

pllim · 2024-04-29T18:01:55Z

astropy/stats/funcs.py

-        from scipy.misc import comb, factorial
+    if not HAS_SCIPY:
+        raise ModuleNotFoundError(
+            "scipy is required for kuiper_false_positive_probability"


This changes the message from:

ImportError: cannot import name 'comb' from 'scipy.<module>'

to:

ModuleNotFoundError: scipy is required for kuiper_false_positive_probability

I'll defer to @larrybradley whether this needs a change log or not.

Technically, it is an API change, but I think it's an inconsequential one. I doubt anyone is running this code and checking for the error type or message.

OK I think everyone else think we do not need a change log, so let's go with the majority. Thanks for the feedback!

pllim · 2024-04-29T22:02:09Z

There are already 4 approvals. Are we still waiting for someone in particular? If not, a maintainer can merge. Thanks!

neutrinoceros · 2024-04-30T07:46:23Z

Thanks @eerovaher for adding a bullet list to track approvals, and sorry I didn't anticipate that this PR would ping so many maintainers at once.

pllim · 2024-04-30T14:04:25Z

Re: pings -- It follows the rules at https://github.com/astropy/astropy/blob/main/.github/CODEOWNERS (based on opt-in by individual core maintainers). FYI.

matteobachetti · 2024-04-30T15:12:49Z

FWIW, changes in io.misc are fine

nstarman · 2024-06-03T15:37:52Z

Do we have enough approvals / has it been long enough waiting / can we merge this in?

pllim · 2024-06-03T16:08:40Z

It's been so long... does it need a rebase?

neutrinoceros · 2024-06-03T18:07:05Z

Dunno, but better safe than sorry: rebased !

pllim · 2024-06-03T22:10:03Z

Can you please rebase one more time? Thanks for your patience!

neutrinoceros · 2024-06-04T06:49:03Z

done

pllim · 2024-06-04T17:49:08Z

Thanks!

neutrinoceros requested review from a team, WilliamJamieson, astrofrog, dhomeier, larrybradley, matteobachetti, saimn and taldcroft as code owners April 29, 2024 07:32

neutrinoceros changed the title ~~ENH: replace try/except ImportError blocks with if HAS_X~~ ENH: replace try/except ImportError blocks with if HAS_X Apr 29, 2024

github-actions bot added table io.fits time io.ascii nddata modeling utils io.misc stats convolution unified-io visualization timeseries labels Apr 29, 2024

neutrinoceros mentioned this pull request Apr 29, 2024

DEP: bump minimal supported IPython version to 7.32, declare ipykernel and ipywidgets as optional deps, cleanup #16354

Merged

1 task

neutrinoceros force-pushed the dep/hunting_except_importerror branch from 33cbeae to 7c4dabc Compare April 29, 2024 08:19

astrofrog reviewed Apr 29, 2024

View reviewed changes

neutrinoceros force-pushed the dep/hunting_except_importerror branch from 7c4dabc to 3245469 Compare April 29, 2024 08:29

taldcroft approved these changes Apr 29, 2024

View reviewed changes

larrybradley approved these changes Apr 29, 2024

View reviewed changes

pllim added this to the v7.0.0 milestone Apr 29, 2024

pllim reviewed Apr 29, 2024

View reviewed changes

pllim added the Refactoring label Apr 29, 2024

eerovaher approved these changes Apr 29, 2024

View reviewed changes

pllim reviewed Apr 29, 2024

View reviewed changes

mhvk approved these changes Apr 29, 2024

View reviewed changes

astropy/utils/xml/writer.py Show resolved Hide resolved

pllim reviewed Apr 29, 2024

View reviewed changes

pllim added the no-changelog-entry-needed label Apr 29, 2024

neutrinoceros force-pushed the dep/hunting_except_importerror branch from e539b5f to 7893fe2 Compare June 3, 2024 18:06

ENH: replace try/except ImportError blocks with if HAS_X

c6aafdf

neutrinoceros force-pushed the dep/hunting_except_importerror branch from 7893fe2 to c6aafdf Compare June 4, 2024 06:49

pllim merged commit 530fd5d into astropy:main Jun 4, 2024

neutrinoceros deleted the dep/hunting_except_importerror branch June 4, 2024 19:51

Uh oh!

Conversation

neutrinoceros commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Approvals by sub-package

Uh oh!

github-actions bot commented Apr 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

larrybradley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neutrinoceros commented Apr 29, 2024

Uh oh!

pllim Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Apr 29, 2024

Uh oh!

neutrinoceros commented Apr 30, 2024

Uh oh!

pllim commented Apr 30, 2024

Uh oh!

matteobachetti commented Apr 30, 2024

Uh oh!

nstarman commented Jun 3, 2024

Uh oh!

pllim commented Jun 3, 2024

Uh oh!

neutrinoceros commented Jun 3, 2024

Uh oh!

pllim commented Jun 3, 2024

Uh oh!

neutrinoceros commented Jun 4, 2024

Uh oh!

pllim commented Jun 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

neutrinoceros commented Apr 29, 2024 •

edited

Loading

pllim Apr 29, 2024 •

edited

Loading