Use external erfa instead of our own python wrappers - #10329

Wow! +12 −49,065

The question of bundling is interesting. Basically astropy core won't work without ERFA, so that would imply a new hard top-level dependency alongside numpy. This seems a bit unbalanced.

I see no reason why it should be any harder to install a specific version of pyerfa as an external dependency than it is to install astropy with it bundled in, if you want to maintain an exact astropy<>pyerfa version match then hard pin the dep at runtime.

Bundling in 50,000 lines of code just to keep "only numpy as a dependency" does imo not reflect the state of the modern python packaging ecosystem and is causing more work than it need to.

@Cadair - fair enough! Just thought it worth a discussion on adding the 2nd hard-requirement after Numpy. We've managed for 10 years to maintain only that one hard requirement.

Just thought it worth a discussion

Always! I don't know enough about the use of erfa in astropy to know how important it is going to be to tie astropy versions to a specific version (rather than a min) or how many other places are likely to use pyerfa (and therefore the probability of a version conflict if pinned). So that's certainly a choice that needs to be made.

Also I am unsure if there is any advantage to keeping it bundled for a release or not as @mhvk suggested was a possibility. What are we gaining by doing that, and if we did that would we use the installed package if it existed?

We've managed for 10 years to maintain only that one hard requirement.

I think the fact that it's been 10 years is why we are able to have this discussion. The state of python packaging has changed (and is still changing) rapidly. With the upcoming massive changes to pip (making it a full graph resolving package manager) I think this will continue to get better and better.

I would be in favor of having it as a new required dependency and not bundle (note that for a while we had pytest as a hard dependency, so we've broken the rule of numpy-only before) - we can certainly pin it to specific versions if we are worried about compatibility. To me, the single biggest benefit will be a noticeable reduction in build time for both users installing from source, as well as in continuous integration. This is a change that can help a little towards reducing our carbon footprint 🌍 😄

I definitely do not want to end up in a situation where astropy has 20 required dependencies in future, so I'm not suggesting this would set a precedent for adding just about anything as a required dependency. But I think this is definitely a very core dependency for us.

PyERFA is a very small dependency, and wheels will be provided, so it is unlikely any users will have issues with installation, and I believe @bsipocz and/or @eteq already checked with the continuum team that they would be happy to include pyerfa as a conda package in defaults so that astropy can continue to be installed from there (I believe this was @eteq's main original objection to not bundling PyERFA in astropy core).

I wrote that we should discuss in part because I both remember from quite a while ago how painful it was to have dependencies and agree that it has changed a great deal. I don't think in itself there is anything against having hard dependencies on the ability to do numerical work in python (numpy) and essential routines for astronomy.

p.s. Looking at current external, the three there -- configobj, jquery and ply -- are all quite small. Must admit I wonder why the largest, jquery, is bundled in, since it is used only to display tables in html and, at nearly 1MB, is not that small (5% of astropy). Possibly we should move to a required, recommended, suggested model. (Erfa has 2.4MB for C routines, 128k for python)

how important it is going to be to tie astropy versions to a specific version (rather than a min) or how many other places are likely to use pyerfa (and therefore the probability of a version conflict if pinned)

Exact pinning is conflict bound to happen. I think we need to research this a bit more before making a decision.

I think generally we will want for every version of astropy to use the latest SOFA/ERFA version. But we can see from our history that those updates do not happen very frequently.

OK, @avalentino, @astrofrog and I made a first PyERFA release, and I now set this one up so that it is being used. So, this is ready for proper review!

p.s. It shaves off only half a minute, but this is for every run, so it will add up...

Would be nice to have review for this... Coverage failure is unrelated.

Hmm... Does that mean the conda recipes (on default and conda-forge) need to be updated too. Not sure where to make a note of that.

This breaks downstream testing (but will be an issue for users, too), and has to be done with a deprecation unless an automatic installation is possible: https://travis-ci.org/github/astropy/astroquery/jobs/706458788

Re: astroquery -- Why isn't it using from astropy.utils.exceptions import ErfaWarning?

good question, I suppose it was just simply not propagated back downstream (as it was the original trigger usage to move it to utils)

But even without importing _erfa, I see the same problem over at ginga too downstream:

Installing collected packages: astropy
Successfully installed astropy-4.2.dev385+g8d1d42999
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/travis/miniconda/envs/test/lib/python3.7/site-packages/astropy/__init__.py", line 104, in <module>
    _check_requirement('erfa', __minimum_erfa_version__)
  File "/home/travis/miniconda/envs/test/lib/python3.7/site-packages/astropy/__init__.py", line 98, in _check_requirement
    raise ImportError(msg)
ImportError: erfa version 1.16.0 or later must be installed to use Astropy. erfa is not installed.

🤔

@pllim - pyerfa is in the dependencies, so it should have been installed. Which command did you run ?

Seems like Ginga is using ASTROPY_VERSION=development from ci-helpers. Larry uses tox and does not see the problem with photutils.

Aha... ci-helpers installs astropy dev with --no-deps. Hmmm... What is the proper fix?

https://github.com/astropy/ci-helpers/blob/4c4a723cb7152cf89bd0cd81e7be182bda7209be/travis/setup_dependencies_common.sh#L825-L834

hahh, it's a new dependency so the stable install is not picking it up. Hmm, then we either need to experiment with the new pip solver that is capable to install new dependencies without updating installed ones, or list pyerfa explicitly for that build.

Is astropy/ci-helpers#451 too hacky?

no, that's the perfect workaround

pip does not update existing deps as long as the version requirement is met. It had been doing that at some point in the past, but I think this was a long time ago.

Sorry to have been causing so much trouble!

No need to be sorry! I think this is a valuable experience for when we want to go crazy with #10420 , #8245 , or beyond. 😸

@saimn - not that long time ago, only the current beta solver works as we need for ci-helpers.

Is there any way to turn off the warning?

UPDATE: Sorry for the noise, I found the answer at https://docs.astropy.org/en/stable/warnings.html.