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

Tox 4.0 compatibility #14139

Merged
merged 2 commits into from Jan 16, 2023
Merged

Tox 4.0 compatibility #14139

merged 2 commits into from Jan 16, 2023

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Dec 7, 2022

Description

This pull request is to address compatibility issues with tox 4.0.x released on 2022-12-07.

Fixes #14143

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • 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 "When to rebase and squash commits".
  • 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?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • 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.

@pllim
Copy link
Member

pllim commented Dec 7, 2022

OK, now the failure is at least consistent with the other repo that is reported at tox-dev/tox#2612

@pllim
Copy link
Member

pllim commented Dec 7, 2022

Doesn't look like it is the .tmp vs .temp. Maybe we pin to tox < 4 for now?

@maxnoe
Copy link
Member Author

maxnoe commented Dec 7, 2022

Unfortunately, we cannot just pin tox, since it comes in via https://github.com/OpenAstronomy/github-actions-workflows/blob/main/.github/workflows/tox.yml

@maxnoe maxnoe changed the title Remove pypi-filter from tox.ini Tox 4.0 compatibility Dec 7, 2022
@mhvk
Copy link
Contributor

mhvk commented Dec 7, 2022

Grumpy side comment: I wonder where this idea came from that it is good to upgrade tox for every CI run... But mostly: thanks for trying to fix this!!

tox.ini Outdated Show resolved Hide resolved
Cadair
Cadair previously requested changes Dec 7, 2022
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

I am pretty sure there is a better option than this by using https://github.com/OpenAstronomy/github-actions-workflows/#toxdeps

I can take a swing tomorrow if nobody else beats me.

@Cadair
Copy link
Member

Cadair commented Dec 7, 2022

Grumpy side comment: I wonder where this idea came from that it is good to upgrade tox for every CI run... But mostly: thanks for trying to fix this!!

We aren't really "upgrading" it we are installing it. The upgrade is really for the other user provided packages.

@maxnoe
Copy link
Member Author

maxnoe commented Dec 7, 2022

I am pretty sure there is a better option than this by using https://github.com/OpenAstronomy/github-actions-workflows
/#toxdeps

I mean now this is about getting it to run with tox 4, if you can pin it to 3 to get the ci running again, great!

tox.ini Show resolved Hide resolved
@pllim pllim mentioned this pull request Dec 7, 2022
10 tasks
@pllim
Copy link
Member

pllim commented Dec 7, 2022

Trying out the pinning at #14142

tox.ini Show resolved Hide resolved
@Cadair Cadair dismissed their stale review December 7, 2022 21:33

Different objectives of the PR.

@pllim pllim added this to the v5.3 milestone Dec 8, 2022
@zacharyburnett
Copy link
Contributor

just chiming in to say the JWST pipeline had a similar issue spacetelescope/jwst#7384

since tox is now using PEP517 to build, the C extensions are not built / linked correctly, but that's another issue; is it possible to force the legacy build without using use_develop?

@gaborbernat
Copy link

This PR should remove the pin her ehttps://github.com/maxnoe/astropy/blob/remove_pypi_filter/.github/workflows/ci_workflows.yml#L53, because otherwise you're not testing against tox 4 :-)

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@maxnoe maxnoe marked this pull request as draft December 8, 2022 15:47
@maxnoe
Copy link
Member Author

maxnoe commented Dec 8, 2022

The remaining failures now are the 32 bit parallel test which fails due to xdist not getting the same tests in each worker process (which I suspect somehow is a tox issue since it passes on tox < 4) and the allowed failures, where there is a scipy issue about a missing optional dependency pooch, which I think is not tox' fault.

@pllim
Copy link
Member

pllim commented Dec 8, 2022

Re: 32-bit job -- On our side here, it is not unreasonable to keep the tox pin for that one job because it is going to die soon anyway. However, potential incompatibility with pytest-xdist might still be real but isn't something we can fix here.

@gaborbernat
Copy link

Re: 32-bit job -- On our side here, it is not unreasonable to keep the tox pin for that one job because it is going to die soon anyway. However, potential incompatibility with pytest-xdist might still be real but isn't something we can fix here.

Likely this is probably related to some env-var missing, or random seeding, would be great if someone could look into finding out what 😊

@dhomeier
Copy link
Contributor

dhomeier commented Dec 8, 2022

The remaining failures now are the 32 bit parallel test which fails due to xdist not getting the same tests in each worker process (which I suspect somehow is a tox issue since it passes on tox < 4) and the allowed failures, where there is a scipy issue about a missing optional dependency pooch, which I think is not tox' fault.

Yes, that's likely just a newly pulled in dependency in scipy-dev that needs to be added.

In #14123 I am still getting a 'utf-8' decode error in the Windows job; that's the only place I found where the allowlist_externals might make a difference, though I could not spot any actual difference in the installed packages in the tox run.

tox.ini Outdated Show resolved Hide resolved
@pllim
Copy link
Member

pllim commented Dec 19, 2022

Hold your horses... Let numpy release first and make sure nothing breaks with it. That is more important. Thanks!

@dhomeier
Copy link
Contributor

Is numpy 1.24 involved here? It is already released btw.

@pllim
Copy link
Member

pllim commented Dec 19, 2022

Yes, we want to make sure CI is green with the new release first without having to also worry about tox upgrade. Thanks for your patience.

@dhomeier
Copy link
Contributor

Will need another rebase to get the numpy 1.24 fix from #14193 in; this should also run with tox 4.0.15 then to resolve tox-dev/tox#2747.

@maxnoe
Copy link
Member Author

maxnoe commented Dec 20, 2022

The dev dependency tests fail with an error related to downloading a scipy test image and the link check docs build fails also, but I cannot really make out why.

@pllim

This comment was marked as resolved.

@pllim
Copy link
Member

pllim commented Dec 23, 2022

OK CI should be green now in main. Please rebase. Thanks!

@maxnoe maxnoe force-pushed the remove_pypi_filter branch 2 times, most recently from 6b480b5 to dfdfac3 Compare January 2, 2023 13:15
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Wonderful that this now works!

tox.ini Outdated Show resolved Hide resolved
Copy link
Contributor

@dhomeier dhomeier left a comment

Choose a reason for hiding this comment

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

Looks great now; +1 for opening an issue on PYTHONHASHSEED, and also to keep track of progress on
astrofrog/tox-pypi-filter#8

@maxnoe
Copy link
Member Author

maxnoe commented Jan 2, 2023

The tests are running, just not in parallel due to fixtures and parametrized Tests using sets with string values, which order depends on the sees and then the pytest xdist test collection fails.

@maxnoe
Copy link
Member Author

maxnoe commented Jan 16, 2023

@dhomeier @mhvk @pllim merge?

@maxnoe
Copy link
Member Author

maxnoe commented Jan 16, 2023

Ok, with that the only change in tox.ini is the change from spaces to commas and the removal of pypi filter

@dhomeier
Copy link
Contributor

I've just re-run the CI to check against the current tox 4.3.1, and could not see any no tox-related failures.

@dhomeier dhomeier added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Jan 16, 2023
@dhomeier
Copy link
Contributor

OK, I have not seen any concerns against removing pypi_filter at this point, so if the last commit passes, let's get this in!

@dhomeier
Copy link
Contributor

same Weekly cron failure on main; thanks for the fix!

@pllim
Copy link
Member

pllim commented Jan 17, 2023

I think this patch is incomplete. Two CircleCI jobs that don't run on PR branches now fail, see:

@dhomeier
Copy link
Contributor

Maybe should have rebased again... or simply missed

pip install pip "tox<4"

as it is only running on main? The failed jobs are running tox-3.28.0.

@pllim
Copy link
Member

pllim commented Jan 18, 2023

Good catch, @dhomeier . Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extra CI Run cron CI as part of PR no-changelog-entry-needed testing 💤 merge-when-ci-passes Do not use: We have auto-merge option now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MNT: Unpin tox<4 when we can
9 participants