Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT: Bumping minimum required astropy version #2602

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Nov 26, 2022

Time to bump the minimum astropy (we've reached the 2 year window) so we can rely on its newer features.

@bsipocz bsipocz added this to the v0.4.7 milestone Nov 26, 2022
@codecov
Copy link

codecov bot commented Nov 26, 2022

Codecov Report

Merging #2602 (c626d8b) into main (f59362b) will increase coverage by 0.13%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2602      +/-   ##
==========================================
+ Coverage   64.21%   64.34%   +0.13%     
==========================================
  Files         130      130              
  Lines       16885    16838      -47     
==========================================
- Hits        10842    10835       -7     
+ Misses       6043     6003      -40     
Impacted Files Coverage Δ
astroquery/alma/core.py 49.44% <0.00%> (+0.27%) ⬆️
astroquery/alma/utils.py 32.35% <ø> (-0.08%) ⬇️
astroquery/cadc/core.py 81.04% <ø> (-0.14%) ⬇️
...roquery/ipac/nexsci/nasa_exoplanet_archive/core.py 57.55% <0.00%> (+0.46%) ⬆️
astroquery/utils/commons.py 75.00% <ø> (-0.15%) ⬇️
astroquery/vo_conesearch/validator/tstquery.py 32.00% <0.00%> (+3.87%) ⬆️
astroquery/vo_conesearch/validator/validate.py 19.69% <0.00%> (+2.27%) ⬆️
astroquery/vo_conesearch/vos_catalog.py 69.51% <0.00%> (+1.11%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bsipocz
Copy link
Member Author

bsipocz commented Nov 26, 2022

Unfortunately with both astropy 4.1 and 4.2 I run into errors like the ones below, so I went ahead and bumped the versions a little bit further to astropy 4.2.1 and numpy 1.18 (the latter is only for good measures):

=============================================== short test summary info ================================================
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/__init__.py - RuntimeWarning: numpy.ufunc size changed, may indicate binary incompatibility. Expected 192 from C header, got 216 ...
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/core.py - AttributeError: type object 'astropy.stats._stats.array' has no attribute '__reduce_cython__'
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/tests/__init__.py - AttributeError: type object 'astropy.stats._stats.array' has no attribute '__reduce_cython__'
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/tests/setup_package.py - AttributeError: type object 'astropy.stats._stats.array' has no attribute '__reduce_cython__'
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/tests/test_astrometry_net.py - AttributeError: type object 'astropy.stats._stats.array' has no attribute '__reduce_cython__'
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/tests/test_astrometry_net.py - AttributeError: type object 'astropy.stats._stats.array' has no attribute '__reduce_cython__'
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/tests/test_astrometry_net_remote.py - AttributeError: type object 'astropy.stats._stats.array' has no attribute '__reduce_cython__'
ERROR ../../.tox/oldestdeps/lib/python3.7/site-packages/astroquery/astrometry_net/tests/test_astrometry_net_remote.py - AttributeError: type object 'astropy.stats._stats.array' has no attribute '__reduce_cython__'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 8 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

@bsipocz
Copy link
Member Author

bsipocz commented Nov 26, 2022

cc @keflavich @andamian @jespinosaar @imbasimba @tomdonaldson @mkelley @pllim - do you have any users you know who is stuck on astropy 4.0 series, or this can be a go-ahead?

@bsipocz bsipocz force-pushed the MAINT_bump_astropy_41 branch from 2544f63 to c626d8b Compare November 27, 2022 04:16
@keflavich
Copy link
Contributor

Good by me.

@jespinosaar
Copy link
Contributor

@bsipocz , we will ask around to check if our users are stuck in that specific version for some reason. Just to know how much time we have to collect this feedback, what is your deadline to merge this? Thanks in advance!

cc @esdc-esac-esa-int

@imbasimba
Copy link
Contributor

@bsipocz I don't foresee any issues with this PR for the ESASky module.

@andamian
Copy link

@bsipocz - I'm not aware of any such users.

@bsipocz
Copy link
Member Author

bsipocz commented Nov 28, 2022

Just to know how much time we have to collect this feedback, what is your deadline to merge this?

@jespinosaar - There is no imminent rush, this is more into the generic maintenance remit than to address anything broken.

@eerovaher
Copy link
Member

The minimum numpy version proposed in this pull request is higher than what astropy 4.2.1 requires: https://github.com/astropy/astropy/blob/v4.2.1/setup.cfg#L31-L32 Is there a reason for that?

Furthermore, the title of this pull request should be updated.

@bsipocz
Copy link
Member Author

bsipocz commented Nov 28, 2022

The minimum numpy version proposed in this pull request is higher than what astropy 4.2.1 requires

It's not how it works, a minimum dependency requirement of a dependency mustn't set our minimum dependencies.

E.g. we aim to follow NEP29/SPEC0 not what astropy supports for the minimum version we do. With that approach we should still support old python versions as some of the oldest deps we declare still supports them (just think about it, if we support 2 years old package X that has a mutual dependency of Y, if they follow NEP29, that would mean a support for 4 year old Y for us, which is not how this all was designed).
So, the minor divergence from NEP29 here is that we run into some build errors with 4.2, and thus the simplest workaround is to require 4.2.1.

@bsipocz bsipocz changed the title MAINT: Bumping minimum required astropy version to 4.1 MAINT: Bumping minimum required astropy version Nov 28, 2022
@pllim
Copy link
Member

pllim commented Nov 28, 2022

do you have any users you know who is stuck on astropy 4.0 series, or this can be a go-ahead?

AFAIK, no, unless @tomdonaldson knows otherwise.

@eerovaher
Copy link
Member

I don't mind bumping the minimum required astropy version to 4.2.1 and I don't mind requiring at least numpy 1.18 either. I'm just trying to understand if there is some particular reason for not allowing numpy 1.17.

@bsipocz
Copy link
Member Author

bsipocz commented Nov 28, 2022

Again, the minimum dependencies are independent. It's irrelevant what astropy does, np 1.18 was released more than 24 months ago in fact almost 3 years now, thus it's the default to bump the minimum requirements to. So we could go even to 1.19...

@eerovaher
Copy link
Member

According to the NEP 29 drop schedule we could even require numpy 1.20.

@bsipocz
Copy link
Member Author

bsipocz commented Nov 28, 2022

The calendar in that document is a bit off, I'm looking at the actual release dates on PyPI. Anyway, I'm not looking into being super strict about the 24 months for most dependencies, and therefore not willing to go newer than 1.18 without an actual technical reason at this time.

@eerovaher
Copy link
Member

I was wondering if there was some technical reason for preferring some particular version of numpy. I am not aware of any such reasons myself.

@bsipocz
Copy link
Member Author

bsipocz commented Nov 29, 2022

There are reasons to drop astropy 4.0, namely to finally get rid of the workarounds due to the changes in io.votable, as well as the unit supports for table initialization (e.g. to be used in #2358).

My reading for NEP29 here is to be more on the on demand case, rather than being proactive (astroquery is used in a lot of research code and not primarily in up-to-date CI-ed downstream libraries). So we could keep living with these workarounds, but think it's time to move on instead given all pinged above gives the thumbs up.

@jespinosaar
Copy link
Contributor

Hi @bsipocz , after some tests we do not foresee any issues changing the astropy versions. Many thanks!

@bsipocz bsipocz merged commit 4af365c into astropy:main Nov 30, 2022
@bsipocz
Copy link
Member Author

bsipocz commented Nov 30, 2022

Thanks all for the thumbs ups, and yay for doing this cleanup!

@bsipocz bsipocz deleted the MAINT_bump_astropy_41 branch April 9, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants