Don't require astropy to be importable at setup.py time by astrofrog · Pull Request #8402 · astropy/astropy

astrofrog · 2019-02-07T11:11:18Z

One of my missions currently is to try and reduce the complexity of the very idiosyncratic infrastructure that we've accumulated over the lifetime of the project. One of these in particular is the fact that until now packages have had to be importable at setup.py time even if they contain C extensions, which requires the convoluted approach of defining the __ASTROPY_SETUP__ builtin and making sure that every single C extension we import is conditional on this variable.

In the past, the reason why astropy needed to be importable is because astropy-helpers and the setup_package.py files tried to import astropy.version at various points in the process, though this then grew to include other files. After a series of pull requests in recent weeks, astropy-helpers should now no longer import the whole package (although packages might still do this to themselves) - instead if it really needs to access the version, then for now it uses the import_file function which allows us to import specific files without importing the whole package. This PR now extends this kind of work to the core package to make sure that when we do need to import other files during the setup process, we use import_file rather than importing e.g. astropy.version directly.

I should note the following changes in particular:

For the WCS module, I had to rearrange one of the files to facilitate this approach, but overall this doesn't require too many changes, mostly just combining two files.
We formerly imported astropy in the setup.py file to get __minimum_numpy_version__, but this PR changes that to define the numpy requirement properly in setup.cfg. This does mean duplication for now, but I would argue that since we define the requirement properly in setup.cfg/setup.py, we should consider getting rid of the definition of __minimum_numpy_version__ and the check for the numpy version in astropy/__init__.py, since that is no longer really necessary (when astropy is installed with pip or conda, a compatible version of numpy will be installed)
Currently I raise an exception in astropy/__init__.py if astropy is imported during the setup process. I had intended for that to just be a temporary thing to make sure this all works properly, but it might actually just make sense to keep it to prevent regressions.

Note that the 32-bit CircleCI build will fail but that's just because for some reason it uses astropy-helpers from master not this PR, so let's just ignore that failure.

Cadair · 2019-02-07T11:30:34Z

🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥

astrofrog · 2019-02-07T11:33:08Z

There are a few issues with the builds that don't rely on setup.py commands, so I'll investigate that shortly, but I think this is related to other changes in astropy-helpers (related to build_ext). So feel free to review the PR as if there were no CI failures :)

Note to self: the issue is related to pypa/setuptools#1317 - that is because we import SetuptoolsBuildExt in astropy-helpers and that triggers a cython import in setuptools which fails as setup_requires hasn't been triggered yet. I'm not sure why this isn't an issue when using setup.py directly though, just pip install .. I think the solution is going to be to install the setup_requires dependencies ourselves in astropy-helpers.

pllim · 2019-02-07T14:34:50Z

Nope.

Traceback (most recent call last):
  File "setup.py", line 29, in <module>
    setup(setup_requires=setup_requires)
  File "/root/project/astropy_helpers/astropy_helpers/setup_helpers.py", line 93, in setup
    version = generate_version_py()
  File "/root/project/astropy_helpers/astropy_helpers/version_helpers.py", line 312, in generate_version_py
    version_module = get_pkg_version_module(packagename)
  File "/root/project/astropy_helpers/astropy_helpers/version_helpers.py", line 385, in get_pkg_version_module
    return __import__(packagename + '.version', fromlist=[''])
  File "/root/project/astropy/__init__.py", line 14, in <module>
    raise Exception("Can't import astropy at setup time")
Exception: Can't import astropy at setup time
Exited with code 1

astrofrog · 2019-02-07T14:42:03Z

@pllim - what did you do, and is your astropy_helpers submodule up to date? It looks like you are using an old version of astropy-helpers. Unless you are referring to one of the CI results, in which case as mentioned above the 32-bit failure is expected.

pllim · 2019-02-07T14:45:17Z

Oh... I didn't read the note about 32 bit, I just saw "feel free to review the PR as if there were no CI failures". Besides 32-bit, Travis, html-docs, and mpldev also failed. Is that also expected?

drdavella · 2019-02-07T15:19:45Z

Just want to say that this is my favorite PR.

nden · 2019-02-07T15:20:35Z

The WCS changes look fine. Though I didn't install and test locally.

astrofrog · 2019-02-07T15:38:50Z

@pllim - for now yes, I need astropy/astropy-helpers#452 for things to work properly.

drdavella · 2019-02-07T15:44:13Z

 # LOCAL
 from astropy import log
 from astropy.io import fits
-from . import _docutil as __


Not really a review comment, but I never understood the rationale for importing things this way. Is there a good reason for using __ as the module name?

I have no idea and I found it veeeery confusing

as a way to show it's super private, and no one outside should use any of that functionality? If that's not the purpose, then I'm confused as well. 😕

bsipocz · 2019-02-07T22:47:05Z

I would say this deserves a mention in the changelog under "other changes"

bsipocz

Overall looks very neat. I had only one minor comment, and also would like to see a full green CI before approval and merging.

bsipocz · 2019-02-07T22:59:59Z

-        raise
-    else:
-        _wcs = None
+from . import docstrings as docutils


can we call it something else then docutils? Not that I plan to spend much time around in wcs, but it always confuses me when a name of an existing package is used as an alias for something else.

codecov · 2019-02-08T12:18:23Z

Codecov Report

Merging #8402 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8402      +/-   ##
==========================================
+ Coverage   86.78%   86.79%   +0.01%     
==========================================
  Files         387      386       -1     
  Lines       58123    58090      -33     
  Branches     1060     1060              
==========================================
- Hits        50440    50422      -18     
+ Misses       7068     7053      -15     
  Partials      615      615

Impacted Files	Coverage Δ
astropy/wcs/wcs.py	`78.1% <100%> (+0.27%)`	⬆️
astropy/wcs/__init__.py	`66.66% <100%> (+16.66%)`	⬆️
astropy/config/configuration.py	`83.39% <100%> (+0.25%)`	⬆️
astropy/coordinates/earth.py	`75.84% <100%> (+0.75%)`	⬆️
astropy/convolution/__init__.py	`100% <100%> (+37.5%)`	⬆️
astropy/samp/hub.py	`77.52% <0%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

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

astrofrog · 2019-02-08T15:45:48Z

The CI is now passing! 🎉 - this is now ready for final reviewing (I think we should make sure it has several approvals before merging though as it's a reasonably significant change)

pllim

Definitely cleaner! And the changes does not seem to break wcs docstring magic.

Cadair

lgtm, but what do I know 😛

nden

lgtm

bsipocz

It all looks good to me, and all my concerns were addressed, too.

eteq

I also think this is a very good idea, but I have one concern (well, technically two, but it's really sort of an "either do A or if not, change B" thing) about the exception-raising - see inline.

One other related thing which doesn't necessarily need to hold up the PR but we probably want to keep in mind given these changes: We also use the _ASTROPY_SETUP_ trick in the package template right now, right? Do we want to it from there as well? It's possible that some affiliated package depends on this in some subtle way (a la the wcs change required here), but we probably want to discourage this at least for new packages moving forward, at least? (If not also help fix old ones...)

astrofrog · 2019-02-20T16:32:55Z

@eteq - good catch! Astropy needs to be importable by affiliated packages when they run python setup.py test, so we need a dedicated variable here, not _ASTROPY_SETUP_. I've fixed this in the latest commit.

…thout importing the rest of astropy

…be importable at build-time

…UP_ to prevent imports at setup time

bsipocz · 2019-02-20T21:25:02Z

I wonder whether there is any long term open issue that this would close :) We need to keep on eye on them when triaging and close them :)

eteq · 2019-02-20T21:27:14Z

I restarted the docs build because it looked unrelated but want to be sure. Other than that this looks good, but just to confirm - @astrofrog , did you mean to reject my suggestion of a re-classing of the exception, or just miss it above? (It's ok either way, I just want to make sure it was intended and not missed since I buried it in an inline comment with some other stuff...)

bsipocz · 2019-02-20T21:35:34Z

@eteq the docs build could fail now due to the linkchecking. While it's being revised separately any failures related to that should be ignored.

Co-Authored-By: astrofrog <[email protected]>

astrofrog · 2019-02-21T11:15:59Z

@eteq - I approved your suggestion, sorry I missed that before!

astrofrog added Refactoring no-changelog-entry-needed labels Feb 7, 2019

astrofrog added this to the v3.2 milestone Feb 7, 2019

astrofrog requested review from Cadair, bsipocz, drdavella, eteq, mhvk, nden and taldcroft February 7, 2019 11:11

pllim added the installation label Feb 7, 2019

drdavella reviewed Feb 7, 2019

View reviewed changes

bsipocz reviewed Feb 7, 2019

View reviewed changes

astrofrog mentioned this pull request Feb 8, 2019

Updated astropy-helpers to latest developer version #8403

Merged

astrofrog force-pushed the no-require-import-setup branch from e1cdda3 to 3db2ff6 Compare February 8, 2019 12:18

pllim approved these changes Feb 8, 2019

View reviewed changes

Cadair approved these changes Feb 8, 2019

View reviewed changes

nden approved these changes Feb 8, 2019

View reviewed changes

bsipocz approved these changes Feb 8, 2019

View reviewed changes

eteq requested changes Feb 20, 2019

View reviewed changes

Comment thread astropy/__init__.py Resolved

astrofrog mentioned this pull request Feb 20, 2019

List of things to include in next astropy-helpers/package template updates astropy/astropy-tools#108

Closed

7 of 10 tasks

astrofrog force-pushed the no-require-import-setup branch from 2a48694 to b0d2899 Compare February 20, 2019 16:33

astrofrog added 12 commits February 20, 2019 17:03

Remove astropy import from setup.py

2bca362

Make it so that astropy.wcs.setup_package can be imported and used wi…

72ab85b

…thout importing the rest of astropy

Make astropy._erfa.setup_package work properly without importing astropy

929c75f

Remove instances of _ASTROPY_SETUP_ since astropy no longer needs to …

0a84063

…be importable at build-time

Don't specify module name when using import_file

247486f

Don't hard-code debug flag

573749d

Define default setup_requires and install_requires in setup.cfg

ea22f7f

Prevent import of astropy at setup time

a1e311c

Don't include -q flag on CircleCI

6f2f0b6

Rename imported module name

d6d8879

Add a comment about the _ASTROPY_SETUP_ check in __init__.py

908bfbe

Use a dedicated variable _ASTROPY_CORE_SETUP_ instead of _ASTROPY_SET…

b884da9

…UP_ to prevent imports at setup time

astrofrog force-pushed the no-require-import-setup branch from b0d2899 to b884da9 Compare February 20, 2019 17:03

eteq approved these changes Feb 20, 2019

View reviewed changes

Change exception type

58a47db

Co-Authored-By: astrofrog <[email protected]>

astrofrog merged commit 5cec111 into astropy:master Feb 21, 2019

astrofrog mentioned this pull request Dec 2, 2019

Make astropy not be imported during the setup process astrofrog/astropy#82

Closed

neutrinoceros mentioned this pull request May 3, 2026

RFC: Rearrange OSError logic so it makes sense #19636

Closed

Conversation

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

Uh oh!

Uh oh!

Cadair commented Feb 7, 2019

Uh oh!

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

Uh oh!

Uh oh!

pllim commented Feb 7, 2019

Uh oh!

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

Uh oh!

Uh oh!

pllim commented Feb 7, 2019

Uh oh!

drdavella commented Feb 7, 2019

Uh oh!

nden commented Feb 7, 2019

Uh oh!

astrofrog commented Feb 7, 2019

Uh oh!

drdavella Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

astrofrog Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

bsipocz Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Feb 7, 2019

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

astrofrog commented Feb 8, 2019

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Cadair left a comment

Choose a reason for hiding this comment

Uh oh!

nden left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

eteq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astrofrog commented Feb 20, 2019

Uh oh!

bsipocz commented Feb 20, 2019

Uh oh!

eteq commented Feb 20, 2019

Uh oh!

bsipocz commented Feb 20, 2019

Uh oh!

astrofrog commented Feb 21, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

astrofrog commented Feb 7, 2019 •

edited

Loading

astrofrog commented Feb 7, 2019 •

edited

Loading

astrofrog commented Feb 7, 2019 •

edited

Loading

codecov Bot commented Feb 8, 2019 •

edited

Loading