ENH: replace `try/except ImportError` blocks with `if HAS_X` #16353

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

@@ -899,14 +900,9 @@ def _is_dask_array(data):
    We avoid importing dask unless it is likely it is a dask array,
    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

neutrinoceros requested review from astrofrog, larrybradley, taldcroft, a team, WilliamJamieson, matteobachetti, saimn and dhomeier 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

ENH: replace try/except ImportError blocks with if HAS_X

3245469

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

simplify docstring

e539b5f

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: replace `try/except ImportError` blocks with `if HAS_X` #16353

ENH: replace `try/except ImportError` blocks with `if HAS_X` #16353

neutrinoceros commented Apr 29, 2024 •

edited

github-actions bot commented Apr 29, 2024

astrofrog Apr 29, 2024

astrofrog Apr 29, 2024

neutrinoceros Apr 29, 2024

astrofrog Apr 29, 2024

neutrinoceros Apr 29, 2024

taldcroft left a comment

larrybradley left a comment

pllim Apr 29, 2024

eerovaher Apr 29, 2024

eerovaher left a comment

pllim left a comment

mhvk left a comment

neutrinoceros commented Apr 29, 2024

pllim Apr 29, 2024 •

edited

larrybradley Apr 29, 2024

pllim Apr 29, 2024

pllim commented Apr 29, 2024

neutrinoceros commented Apr 30, 2024

pllim commented Apr 30, 2024

matteobachetti commented Apr 30, 2024

ENH: replace try/except ImportError blocks with if HAS_X #16353

Are you sure you want to change the base?

ENH: replace try/except ImportError blocks with if HAS_X #16353

Conversation

neutrinoceros commented Apr 29, 2024 • edited

Description

Approvals by sub-package

github-actions bot commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taldcroft left a comment

Choose a reason for hiding this comment

larrybradley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerovaher left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

neutrinoceros commented Apr 29, 2024

pllim Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Apr 29, 2024

neutrinoceros commented Apr 30, 2024

pllim commented Apr 30, 2024

matteobachetti commented Apr 30, 2024

ENH: replace `try/except ImportError` blocks with `if HAS_X` #16353

ENH: replace `try/except ImportError` blocks with `if HAS_X` #16353

neutrinoceros commented Apr 29, 2024 •

edited

pllim Apr 29, 2024 •

edited