Speed up wcsaxes import by avoiding unnecessary pytest import by lpsinger · Pull Request #10302 · astropy/astropy

lpsinger · 2020-05-04T19:53:49Z

In astropy.visualization.wcsaxes, do not import pytest unless we are actually running the tests. Pytest is a fairly heavyweight import, so it should not be imported unless it's actually needed.

mhvk

Yes, this simply seems wrong. I'm confident enough to approve, but it would be good for @astrofrog to review, since he added it here.

mhvk · 2020-05-04T20:19:13Z

@lpsinger - if you did notice other cases, and have time, definitely tell where or open other PRs.

lpsinger · 2020-05-04T20:19:13Z

Unfortunately, the conftest.py doesn't work. I'm trying a few different strategies here.

mhvk · 2020-05-04T20:20:09Z

Might it work in tests/__init__.py?

Approval was premature

mhvk · 2020-05-04T20:26:20Z

Hmm, I think I'm starting to see the problem (never think @astrofrog did strange looking things for no reason..). pytest probably traverses the tree and tries to import things, just to look for tests.

But perhaps one can avoid loading pytest by trying to import matplotlib in __init__.py and if that fails do the pytest.importorskip step? By that time, the user is most likely trying to import something from wcsaxes anyway, so if matplotlib is not present, they are getting a failure regardless and the time doesn't matter so much.

Aside: pytest used to be just a single file which we shipped, hardly a heavy dependence!

mhvk · 2020-05-04T20:27:16Z

p.s. I cancelled travis to save some cycles, since you wrote things didn't work.

pep8speaks · 2020-05-04T20:39:31Z

Hello @lpsinger 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-05-04 21:18:18 UTC

lpsinger · 2020-05-04T20:40:53Z

@mhvk, @bsipocz, I force-pushed a new implementation. Please take a look and send feedback. If you like it, I'll clean it up, make the same change for all other impacted modules, and add a changelog entry.

In `astropy.visualization.wcsaxes`, do not import pytest unless we are actually running the tests. Pytest is a fairly heavyweight import, so it should not be imported unless it's actually needed.

pllim · 2020-05-04T21:27:38Z

astropy/visualization/wcsaxes/__init__.py

+
+if tests.RUNNING:
    import pytest
    pytest.importorskip("matplotlib")


Wait a minute. Why is this even here? It should have been in the individual test modules under tests. Looks like we inherited this from the package we absorbed in #5496. @astrofrog , do you remember why this is at subpackage init level and not the tests?

I suspect that this is because wcsaxes is unique in Astropy in that it subclasses many different Matplotlib subclasses, and so the conditional imports of the kind in astropy.visualization.mpl_normalize were not appropriate:

try: import matplotlib # pylint: disable=W0611 from matplotlib.colors import Normalize from matplotlib import pyplot as plt except ImportError: class Normalize: def __init__(self, *args, **kwargs): raise ImportError('matplotlib is required in order to use this ' 'class.')

I suppose it was a convenient way to make sure mpl is needed for the whole module and skip it at collection time at the earliest and quickest way possible. Fully agreed that it should not be used like this.

pllim

I have a feeling that this is not the correct solution. See my comment above. Thanks for bringing this to our attention!

mhvk · 2020-05-04T21:34:39Z

@lpsinger - that feels too hacky, modifying some almost global state. I also worry that things like pytest astropy/visualization will not be covered. My suggestion above might be better, but I agree with @pllim that at this point it would be good to get @astrofrog's opinion, since he almost certainly struggled with this when moving wcsaxes over here.

Also, which other modules have things like this? Maybe some of those can more easily be changed? Because you are definitely right that we should not be importing pytest anywhere but in tests directories!

lpsinger · 2020-05-04T21:35:59Z

Also, which other modules have things like this? Maybe some of those can more easily be changed? Because you are definitely right that we should not be importing pytest anywhere but in tests directories!

Happily, this is the only such module in Astropy itself.

pllim · 2020-05-05T00:41:09Z

I feel like the more correct solution would be something like #10260 but it is a lot more work and is borderline controversial to some people.

I mean, if we make yaml, asdf, etc go through strict import checks in io.misc, why not here as well? matplotlib is just as optional as those packages.

mhvk · 2020-05-05T01:56:41Z

Agree with the sentiment, though there are lots of modules here which use it, and having the whole module indented becomes a bit much! But perhaps there are cleverer solutions?

pllim · 2020-05-05T02:06:44Z

Well, in the PR for asdf linked above, the dependency namespace is replaced with generic junk, which would make the import work fine without dependency installed, but you get exception when you actually try to use the stuff for real. So the tradeoff is no error on import but you don't get told you are missing a dependency until later. 🤷 This game is hard to win.

astropy-bot · 2020-10-07T08:01:25Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

lpsinger · 2021-04-07T13:32:23Z

I see that this has been re-milestoned several times. Would you like me to rebase it?

pllim · 2021-04-07T13:38:33Z

@lpsinger , thanks for pinging us. Personally, I am not convinced that this is the correct solution.

It is weird that the pytest.importorskip("matplotlib") is in __init__.py of a non-test code, so that definitely needs to be fixed, but not like this. Perhaps @astrofrog or @Cadair can provide some insight.

astrofrog · 2021-04-07T13:50:46Z

I wonder if we could use

astropy/setup.cfg

Line 145 in ba283e7

doctest_subpackage_requires =

to prevent issues with pytest when matplotlib is not installed? (Not at computer so can't check right now)

lpsinger · 2021-04-07T13:53:58Z

FWIW, I tried putting the pytest.importorskip in a conftest.py module, and it didn't work.

lpsinger · 2021-04-07T13:59:18Z

I wonder if we could use

astropy/setup.cfg

Line 145 in ba283e7

doctest_subpackage_requires =

to prevent issues with pytest when matplotlib is not installed? (Not at computer so can't check right now)

Just tried that. Didn't work.

pllim · 2021-04-07T14:53:05Z

What about moving pytest.importorskip("matplotlib") to each of the affected test modules, and deleting it from __init__.py? Sure, it is more duplication but it is more correct, no?

lpsinger · 2021-04-07T14:58:27Z

What about moving pytest.importorskip("matplotlib") to each of the affected test modules, and deleting it from __init__.py? Sure, it is more duplication but it is more correct, no?

That would not work. The import errors occur during collection, not while running the tests.

pllim · 2021-04-07T15:31:55Z

The import errors occur during collection

Ah, right, that rings a faraway bell now.

Sigh... How does io.misc do it? I mean, they use optional dependencies too and I don't see pytest imported in astropy/io/misc/asdf/__init__.py

github-actions · 2021-09-04T05:37:21Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 1 year, 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

github-actions · 2021-10-05T05:36:02Z

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

astropy-bot bot added visualization visualization.wcsaxes labels May 4, 2020

mhvk added no-changelog-entry-needed testing labels May 4, 2020

mhvk added this to the v4.0.2 milestone May 4, 2020

mhvk added the Bug label May 4, 2020

mhvk previously approved these changes May 4, 2020

View reviewed changes

lpsinger force-pushed the pytest-speed-up-import branch from d0df5e6 to 4868eda Compare May 4, 2020 20:39

Speed up wcsaxes import by avoiding unnecessary pytest import

43fea65

In `astropy.visualization.wcsaxes`, do not import pytest unless we are actually running the tests. Pytest is a fairly heavyweight import, so it should not be imported unless it's actually needed.

lpsinger force-pushed the pytest-speed-up-import branch from 4868eda to 43fea65 Compare May 4, 2020 21:18

lpsinger changed the title ~~WIP: Speed up astropy imports by moving pytest skips to conftest.py~~ Speed up wcsaxes import by avoiding unnecessary pytest import May 4, 2020

pllim reviewed May 4, 2020

View reviewed changes

pllim requested changes May 4, 2020

View reviewed changes

This was referenced May 7, 2020

TST: Globally ignore import error during test collection for doctest #10325

Closed

Of required and optional dependencies #10342

Open

pllim requested a review from astrofrog May 11, 2020 21:45

astrofrog modified the milestones: v4.0.2, v4.0.3, v4.0.4 Oct 10, 2020

bsipocz modified the milestones: v4.0.4, v4.0.5 Nov 25, 2020

eteq modified the milestones: v4.0.5, v4.0.6 Mar 18, 2021

astrofrog modified the milestones: v4.0.6, v4.0.7 Aug 24, 2021

github-actions bot added the Close? Tell stale bot that this issue/PR is stale label Sep 3, 2021

github-actions bot added the closed-by-bot Closed by stale bot label Oct 5, 2021

github-actions bot closed this Oct 5, 2021

Uh oh!

Conversation

lpsinger commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 4, 2020

Uh oh!

lpsinger commented May 4, 2020

Uh oh!

mhvk commented May 4, 2020

Uh oh!

mhvk commented May 4, 2020

Uh oh!

mhvk commented May 4, 2020

Uh oh!

pep8speaks commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-05-04 21:18:18 UTC

Uh oh!

lpsinger commented May 4, 2020

Uh oh!

pllim May 4, 2020

Choose a reason for hiding this comment

Uh oh!

lpsinger May 4, 2020

Choose a reason for hiding this comment

Uh oh!

bsipocz May 5, 2020

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 4, 2020

Uh oh!

lpsinger commented May 4, 2020

Uh oh!

pllim commented May 5, 2020

Uh oh!

mhvk commented May 5, 2020

Uh oh!

pllim commented May 5, 2020

Uh oh!

astropy-bot bot commented Oct 7, 2020

Uh oh!

lpsinger commented Apr 7, 2021

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

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

Uh oh!

Uh oh!

lpsinger commented Apr 7, 2021

Uh oh!

lpsinger commented Apr 7, 2021

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

lpsinger commented Apr 7, 2021

Uh oh!

pllim commented Apr 7, 2021

Uh oh!

github-actions bot commented Sep 4, 2021

Uh oh!

github-actions bot commented Oct 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lpsinger commented May 4, 2020 •

edited

Loading

pep8speaks commented May 4, 2020 •

edited

Loading

astrofrog commented Apr 7, 2021 •

edited

Loading