Refactor optional dependency checking by adrn · Pull Request #11490 · astropy/astropy

adrn · 2021-04-07T02:13:35Z

This PR centralizes the definition of HAS_x optional dependency package import checking, such as HAS_matplotlib. Now that astropy requires Python 3.7+, we can make use of lazy imports as implemented in PEP 0562 and as demonstrated by @pllim in this comment. This implementation is similar to what @pllim suggested, but with some extra flexibility to support flags that check subpackage imports that have different behavior than the top-level package import (e.g., matplotlib.pyplot). The actual implementation is in the new file astropy/utils/compat/optional_deps.py and was added in the first two commits.

I decided to go with using the actual package names in the flag names (e.g., HAS_matplotlib not HAS_MATPLOTLIB) because there are cases where the case matters for a package name, like PIL or IPython.

This fixes #4630, and is relevant for a comment in #11361

cc @nstarman

taldcroft

Nice! Just a couple of comments on the actual code (I didn't review the rest pending one of my comments to use upper case).

taldcroft

Looks great, I went through the whole list and didn't see any problems. The only thing left are the docs: https://docs.astropy.org/en/stable/development/testguide.html#tests-requiring-optional-dependencies

adrn · 2021-04-07T14:56:33Z

Great, thanks @taldcroft for having the patience to look through all of the changes! I updated the docs, but currently astropy.utils.compat is not automodapi'd, so I don't link explicitly to the module with the list of supported flags.

taldcroft

Looks good from my perspective, thanks!!

adrn · 2021-04-07T15:17:43Z

Hm, did RTD fail because I used [ci skip]? Maybe @pllim knows?

pllim · 2021-04-07T15:25:09Z

RTD failure might be transient... try another push (forced or not)

mhvk · 2021-04-07T18:00:12Z

Just thinking that if we are going to change things around, doing it once only might be better... But agreed that it really is out of scope and we might as well stick with this nice improvement! After all, it is not as if this precludes the scheme I suggested.

adrn · 2021-04-07T18:19:00Z

OK, just made a small change based on a suggestion from @mhvk - assuming the relevant tests pass, I think this is ready!

pllim

This is great! Certainly seems more sane than before.

Just curious: Did you time the astropy import to make sure this does not have bad performance side effects? Sometimes we get complains that importing astropy is too slow as it is.

Some minor comments below.

pllim · 2021-04-07T19:43:57Z

-    import yaml  # noqa
-    HAS_YAML = True
-except ImportError:
-    HAS_YAML = False


Hmm... What was the original purpose of this import? Was it to let people use astropy.io.ascii.ui.HAS_YAML?

It's not documented anywhere that I could find as public API -- possible vestigial?

pllim · 2021-04-07T20:28:11Z

+from astropy.utils.compat.optional_deps import HAS_SORTEDCONTAINERS

-try:
+if HAS_SORTEDCONTAINERS:


Will renaming HAS_SOCO to HAS_SORTEDCONTAINERS break anything downstream? Maybe @taldcroft knows.

I don't think these things were ever exposed as public API... Also don't see any reference to this in docs. Also don't see any uses of this in other github projects https://github.com/search?p=1&q=HAS_SOCO&type=Code

pllim · 2021-04-07T20:32:50Z

-test if the dependencies are not present. The following example shows
-how this should be done::
+For tests that test functions or methods that require optional dependencies
+(e.g. Scipy), pytest should be instructed to skip the test if the dependencies


Nitpick.

Suggested change

(e.g. Scipy), pytest should be instructed to skip the test if the dependencies

(e.g., Scipy), pytest should be instructed to skip the test if the dependencies

OK, but I'm skipping CI on this!

pllim · 2021-04-07T20:34:01Z

+are not present, as the Astropy tests should succeed even if an optional
+dependency is not present. Astropy provides a list of boolean flags that test


Capitalized "Astropy" is for the project, not the package.

Suggested change

are not present, as the Astropy tests should succeed even if an optional

dependency is not present. Astropy provides a list of boolean flags that test

are not present, as the ``astropy`` tests should succeed even if an optional

dependency is not present. ``astropy`` provides a list of boolean flags that test

Ah, yes, @lglattly should shake her finger at me :) -- skipping CI for this too...but I'll wait for the last Cron test to finish.

adrn · 2021-04-07T20:53:09Z

Failures in ppc64le and s390x look unrelated. Other tests (except link check) pass. I think it's overkill to run the cron tests again, and I have some doc changes to push from @pllim, so I will [ci skip] - sound ok?

adrn · 2021-04-07T20:58:40Z

Well, I guess #11091 got merged, so never mind!

pllim · 2021-04-07T21:00:03Z

Sorry... 😬

pllim · 2021-04-07T21:00:40Z

I don't think a rebase warrants more exotic tests, so I removed the Extra CI label.

…_ names separately

matplotlib.pyplot bleach bottleneck bs4 bz2 h5py preserve extreme laziness of imports in connect.py jplephem mpmath, some scipy pandas pkg_resources pytz scipy sortedcontainers yaml fix some accidentally removed imports one more removed import revert cased variable names using actual package names revert error message to standard attribute error oops, typo

…_MATPLOTLIB

pllim

I am still curious about the import timing though I don't expect anything shocking, so approving!

pllim · 2021-04-07T21:17:30Z

Wut...? I saw this error in a different PR too. @saimn , do you know what's going on? Maybe we should just merge #10877 and be done with it...

>       assert is_unedited_config_file(content)
E       AssertionError: assert False
E        +  where False = <function is_unedited_config_file at 0x7f282632ec10>('\n# Use Unicode characters when outputting values, and writing widgets to the\n# console.\nunicode_output = False\n[u...non-critical\nnoncrit_warnings = W03, W06, W07, W09, W10, W15, W17, W20, W21, W22, W27, W28, W29, W41, W42, W48, W50\n')

pllim · 2021-04-07T21:19:47Z

Well, now master has the same failure too. Let's just merge this. There is no reason to delay.

saimn · 2021-04-07T22:28:23Z

That was the changes in the astropy.0.3.cfg test files, before the config PR the code was checking hashes for those files.

adrn requested review from pllim and taldcroft April 7, 2021 02:13

github-actions Bot added convolution coordinates cosmology io.ascii io.fits io.misc io.registry modeling stats table testing time timeseries uncertainty units utils visualization wcs labels Apr 7, 2021

adrn added the Affects-dev PRs and issues that do not impact an existing Astropy release label Apr 7, 2021

adrn added this to the v4.3 milestone Apr 7, 2021

taldcroft requested changes Apr 7, 2021

View reviewed changes

Comment thread astropy/utils/compat/optional_deps.py Outdated

Comment thread astropy/utils/compat/optional_deps.py Outdated

adrn force-pushed the utils/optional-deps branch from 910cf6b to 22f5c26 Compare April 7, 2021 12:19

taldcroft reviewed Apr 7, 2021

View reviewed changes

taldcroft approved these changes Apr 7, 2021

View reviewed changes

pllim removed the Affects-dev PRs and issues that do not impact an existing Astropy release label Apr 7, 2021

adrn force-pushed the utils/optional-deps branch from 5929e85 to 621b32a Compare April 7, 2021 18:18

pllim reviewed Apr 7, 2021

View reviewed changes

pllim removed the Extra CI Run cron CI as part of PR label Apr 7, 2021

adrn added 10 commits April 7, 2021 17:00

add optional dependency variable store

d3dab8c

modify to maintain a list of optional package names, and make the HAS…

40ce4e3

…_ names separately

add support for subpackages too

007cc6f

document new boolean flag for optional dependencies [docs only]

de6976b

minor tweak to wording in docs [ci skip]

9f1bb9e

added a changelog entry

3d5ba5b

silence matplotlib deprecation warning before importing

f71860d

oops, the first import matplotlib actually happens when importing HAS…

18e1cea

…_MATPLOTLIB

import names directly to io.misc expected names

5945357

adrn force-pushed the utils/optional-deps branch from 621b32a to 5945357 Compare April 7, 2021 21:04

nitpicks from @pllim

3837c6d

pllim approved these changes Apr 7, 2021

View reviewed changes

pllim added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Apr 7, 2021

pllim merged commit b478b55 into astropy:main Apr 7, 2021

This was referenced Apr 11, 2021

A general Masked class for Quantity and other ndarray subclasses #11127

Merged

Extend astropy.cosmology.z_at_value to accept array of values #11361

Closed

adrn mentioned this pull request Sep 3, 2021

Fund core project members with Moore Grant 2021 astropy/astropy-project#199

Closed

	(e.g. Scipy), pytest should be instructed to skip the test if the dependencies
	(e.g., Scipy), pytest should be instructed to skip the test if the dependencies

		are not present, as the Astropy tests should succeed even if an optional
		dependency is not present. Astropy provides a list of boolean flags that test

Conversation

adrn commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

adrn commented Apr 7, 2021

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

adrn commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

mhvk commented Apr 7, 2021

Uh oh!

adrn commented Apr 7, 2021

Uh oh!

pllim 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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

adrn commented Apr 7, 2021

Uh oh!

adrn commented Apr 7, 2021

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

saimn commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adrn commented Apr 7, 2021 •

edited

Loading

adrn commented Apr 7, 2021 •

edited

Loading