Skip to content

Update sortedcontainers version requirement#18175

Merged
neutrinoceros merged 1 commit intoastropy:mainfrom
eerovaher:oldest-sortedcontainers
Nov 4, 2025
Merged

Update sortedcontainers version requirement#18175
neutrinoceros merged 1 commit intoastropy:mainfrom
eerovaher:oldest-sortedcontainers

Conversation

@eerovaher
Copy link
Copy Markdown
Member

@eerovaher eerovaher commented May 26, 2025 •
edited
Loading

Original description

The oldest version of sortedcontainers that astropy claims to support is incompatible with Python 3.11 because it imports Sequence and MutableSequence from collections whereas it should be importing them from collections.abc instead. The oldest version that is compatible with Python 3.11 is 2.0.5 (released in 2018).

The reason this is not causing failures in our oldest dependencies CI job is that those jobs use version 2.1.0 despite what our pyproject.toml specifies. I don't have an explanation for the discrepancy.

UPDATE

Turns out that the discrepancy was caused by indirect dependencies. I investigated further (by running uv pip compile --resolution lowest-direct --extra test_all pyproject.toml) and found a few more cases where the oldest version astropy claims to support cannot be tested. I expanded this pull request so that it updates our requirements accordingly at this point in time, but we still don't have a mechanism that would ensure our requirements stay consistent with what we actually test.

UPDATE 2

The required versions of quite a few packages have been updated in other pull requests, so I am again limiting this PR to sortedcontainers. Our oldest dependencies CI job is already using 2.1.0 because that is what hypothesis requires. Claiming to support older versions than that is dangerous because if there are any problems our CI will not catch them. Indeed, versions older than 2.0.5 are known to be incompatible with modern astropy because they are incompatible with Python 3.11, which is the oldest Python modern astropy supports.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@eerovaher eerovaher added this to the v7.2.0 milestone May 26, 2025
@eerovaher eerovaher requested a review from neutrinoceros May 26, 2025 21:46
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 26, 2025

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@eerovaher eerovaher modified the milestones: v7.2.0, v7.1.1 May 26, 2025
@pllim pllim added the Bug label May 27, 2025
@pllim pllim requested a review from a team May 27, 2025 15:19
@pllim
Copy link
Copy Markdown
Member

pllim commented May 27, 2025

If you want to backport, please add a bugfix change log to justify this bump in a backport branch. Thanks!

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented May 28, 2025

CI still got 2.1.0, so, by your argument, we should make that our minimal requirement.
I don't know the reason for this discrepancy either, though it could be clarified by installing a clean env with uv + RUST_LOG=uv=debug, but we've already seen in a previous attempt that this doesn't play nice with tox + tox-uv, so I don't think it's worth the investigation, given sortedcontainers 2.1.0 is 7 years old, which should be plenty.

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented May 28, 2025 •
edited
Loading

nevermind I actually got the explanation: sortedcontainers is a direct dependency to hypothesis, which we only require indirectly via pytest-astropy, so we get the latest version, which imposes sortedcontainers>=2.1.0

https://github.com/HypothesisWorks/hypothesis/blob/8ba690e1f6b3e5c5374cd76b24f5c2f9b25ec907/hypothesis-python/setup.py#L101

@eerovaher
Copy link
Copy Markdown
Member Author

eerovaher commented May 28, 2025

The problem here is that astropy can work just fine with sortedcontainers 2.0.5, but our testing infrastructure requires at least 2.1.0, so we can't really test that astropy works with older sortedcontainers. A naive approach would be to only support sortedcontainers >= 2.1.0, but that is not a robust solution because if hypothesis ever updates their sortedcontainers requirement then the same problem will re-emerge. I'm not sure how to proceed.

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented May 28, 2025

The only robust solution I can see would be to drop pytest-astropy as a (meta)dependency and replace it with its dependencies, but I have no idea how controversial this proposal could feel to other devs.

@pllim
Copy link
Copy Markdown
Member

pllim commented May 29, 2025

Re: dropping pytest-astropy

@astrofrog created that metapackage for astropy so he might have strong opinions.

@astrofrog
Copy link
Copy Markdown
Member

astrofrog commented May 29, 2025

I don't really mind but does this really solve the issue in general? We still don't have control over hypothesis's dependency pinning?

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented May 29, 2025

Indeed we don't, but we gain the ability to pin hypothesis itself, which prevents this issue from resurfacing unexpectedly

@eerovaher eerovaher force-pushed the oldest-sortedcontainers branch from 23f7652 to 354648c Compare June 5, 2025 17:16
@eerovaher eerovaher changed the title Update sortedcontainers requirement Update astropy dependency requirement to be testable Jun 5, 2025
@eerovaher eerovaher changed the title Update astropy dependency requirement to be testable Update astropy dependency requirements to be testable Jun 5, 2025
@eerovaher
Copy link
Copy Markdown
Member Author

eerovaher commented Jun 5, 2025

I used uv pip compile to check if there are any more dependency requirements that we are not testing and turns out there were a few. I've therefore decided to expand this pull request accordingly.

eerovaher
eerovaher commented Jun 5, 2025
View reviewed changes
@eerovaher eerovaher force-pushed the oldest-sortedcontainers branch from 354648c to bcf1efa Compare June 13, 2025 17:02
eerovaher
eerovaher commented Jun 13, 2025
View reviewed changes
pyproject.toml Outdated
"pandas>=2.0",
"sortedcontainers>=1.5.7", # (older versions may work)
"pytz>=2016.10", # (older versions may work)
"sortedcontainers>=2.1.0", # imposed by testing with hypothesis
Copy link
Copy Markdown
Member Author

@eerovaher eerovaher Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look up the logs for the latest run of the oldest dependencies CI job on main you will see that sortedcontainers==2.1.0 is what that job already uses.

eerovaher
eerovaher commented Jun 13, 2025
View reviewed changes
@eerovaher
Copy link
Copy Markdown
Member Author

eerovaher commented Jun 13, 2025

Indeed we don't, but we gain the ability to pin hypothesis itself, which prevents this issue from resurfacing unexpectedly

The reality is that if there is no automated test in a CI job for verifying that our oldest requirements are testable then people will forget to check. And if there is a CI job then it doesn't matter (for these purposes) if hypothesis is a direct dependency or indirect via pytest-astropy.

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Sep 23, 2025

And if there is a CI job then it doesn't matter (for these purposes) if hypothesis is a direct dependency or indirect via pytest-astropy.

I don't think I understand your point here.
Other than that, this looks good to me as is, although a rebase/CI refresh would help confirming it hasn't bitrotted.
Sorry it's been so long, this got buried under a pile of notifications I'm still unwinding now.

@astrofrog astrofrog added this to the v7.1.2 milestone Oct 10, 2025
@astrofrog astrofrog modified the milestones: v7.1.2, v7.2.0 Nov 3, 2025
@astrofrog astrofrog added backport-v7.2.x on-merge: backport to v7.2.x and removed backport-v7.1.x labels Nov 3, 2025
@eerovaher eerovaher force-pushed the oldest-sortedcontainers branch from 9070fc9 to 69ef130 Compare November 3, 2025 14:52
@eerovaher
Copy link
Copy Markdown
Member Author

eerovaher commented Nov 3, 2025

I am limiting this PR to sortedcontainers. Updating the required version to 2.1.0 should not be controversial because it was released in 2018 and it is what our oldest dependencies CI job already uses. Removing the dependency on pytz can be done in a separate pull request.

@eerovaher eerovaher changed the title Update astropy dependency requirements to be testable Update sortedcontainers version requirement Nov 3, 2025
@pllim
Copy link
Copy Markdown
Member

pllim commented Nov 3, 2025

Hmm why all the dev deps failing?

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Nov 3, 2025

Looks like https://pypi.anaconda.org/liberfa/simple/pyerfa/ is down.

@pllim
Copy link
Copy Markdown
Member

pllim commented Nov 3, 2025

Might be transient then. Maybe a rebase would fix it.

@neutrinoceros neutrinoceros force-pushed the oldest-sortedcontainers branch from 69ef130 to c87f045 Compare November 4, 2025 11:35
@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Nov 4, 2025

The index is up again. I rebased this for you Eero (but please let me know if I shouldn't do this again in the future).

@neutrinoceros neutrinoceros mentioned this pull request Nov 4, 2025
Closed
@eerovaher
Update sortedcontainers version requirement
af625c3 
`astropy` claimed to support `sortedcontainers>=1.5.7`, but versions
older than 2.0.5 are incompatible with Python 3.11. This went unnoticed
because `hypothesis`, which is a dependency for `astropy` tests,
requires `sortedcontainers>=2.1.0`. Given that 2.1.0 was released in
2018, declaring that as the oldest supported version is not
controversial.
@eerovaher eerovaher force-pushed the oldest-sortedcontainers branch from c87f045 to af625c3 Compare November 4, 2025 14:01
@eerovaher
Copy link
Copy Markdown
Member Author

eerovaher commented Nov 4, 2025

There is no good reason for removing my signature from the commit.

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Nov 4, 2025

did I do that ? Sorry, I'm not sure what you mean.

@eerovaher
Copy link
Copy Markdown
Member Author

eerovaher commented Nov 4, 2025

The committer has the option of signing the commit. By rebasing you become the committer and my signature gets removed.

@pllim
Copy link
Copy Markdown
Member

pllim commented Nov 4, 2025

FWIW we don't enforce GPG signed commit or that sort of stuff here.

@eerovaher eerovaher marked this pull request as ready for review November 4, 2025 15:23
@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Nov 4, 2025

Ok I guess I cannot fix this unless I fully commit to stealing your identity... I'll just avoid rebasing your PRs then 😅

@neutrinoceros
Copy link
Copy Markdown
Contributor

neutrinoceros commented Nov 4, 2025

Re: dropping pytest-astropy

btw @eerovaher you might want to hop-on to #18783

Anyway, I don't see anything we still need to wait on here, so I'll merge it now. Thanks !

@neutrinoceros neutrinoceros added the dependencies Pull requests that update a dependency file label Nov 4, 2025
@neutrinoceros neutrinoceros merged commit 1721758 into astropy:main Nov 4, 2025
53 of 55 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Nov 4, 2025
@neutrinoceros @meeseeksmachine
Backport PR astropy#18175: Update sortedcontainers version requirement
de95faf
@meeseeksmachine meeseeksmachine mentioned this pull request Nov 4, 2025
Merged
@eerovaher eerovaher deleted the oldest-sortedcontainers branch November 4, 2025 15:28
pllim added a commit that referenced this pull request Nov 4, 2025
@pllim
Merge pull request #18836 from meeseeksmachine/auto-backport-of-pr-18…
54643e3 
…175-on-v7.2.x

Backport PR #18175 on branch v7.2.x (Update `sortedcontainers` version requirement)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@neutrinoceros neutrinoceros neutrinoceros approved these changes

Assignees

No one assigned

Labels

backport-v7.2.x on-merge: backport to v7.2.x Bug dependencies Pull requests that update a dependency file Extra CI Run cron CI as part of PR installation no-changelog-entry-needed

Projects

None yet

Milestone

v7.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@eerovaher @pllim @neutrinoceros @astrofrog