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

Restrict pint test runs #8437

Open
max-sixty opened this issue Nov 10, 2023 · 10 comments · Fixed by #8445
Open

Restrict pint test runs #8437

max-sixty opened this issue Nov 10, 2023 · 10 comments · Fixed by #8445
Labels
CI Continuous Integration tools

Comments

@max-sixty
Copy link
Collaborator

What is your issue?

Pint tests are failing on main — https://github.com/pydata/xarray/actions/runs/6817674274/job/18541677930

E               TypeError: no implementation found for 'numpy.min' on types that implement __array_function__: [<class 'pint.util.Quantity'>]

If we can't fix soon, should we disable?

CC @keewis

@max-sixty max-sixty added the CI Continuous Integration tools label Nov 10, 2023
@TomNicholas
Copy link
Contributor

If we can't fix soon, should we disable?

What's the current feasibility of moving these tests out into pint-xarray? That's technically where they should live eventually imo

@keewis
Copy link
Collaborator

keewis commented Nov 10, 2023

What's the current feasibility of moving these tests out into pint-xarray?

should be possible, but running them in xarray's CI has the advantage of testing at least one duck array, so we immediately get feedback if we're breaking something. Though if we replace them with more generic ones there's nothing that should stop us from moving the pint tests.

@dcherian
Copy link
Contributor

dcherian commented Nov 12, 2023

we switched from numpy 1.24.X to 1.26 4 days ago, but stay at pint 0.20.1 when the latest is 0.22. Presumably some compatibility code for the array_function codepath was removed.

EDIT: removing the pin in #8445 still gave same versions 🤷🏾‍♂️

@keewis
Copy link
Collaborator

keewis commented Nov 12, 2023

For more details about this change, see #7420 (comment) (I totally forgot I had fixed that). The first version to include that fix is indeed 0.22, so we should use that.

@keewis keewis mentioned this issue Nov 12, 2023
2 tasks
@benbovy
Copy link
Member

benbovy commented Nov 12, 2023

Would it be possible to have some opt-in for those tests?

It helped catching some regressions a couple of times during the index refactor so I think they are useful (although I agree more generic tests might be good if possible).

But in general I found that running those tests takes a significant part of running the whole xarray test suite.

@keewis
Copy link
Collaborator

keewis commented Nov 13, 2023

the issue with opt-in is that they might not run at all, which means that any bugs they would have caught will slip by unnoticed. However, those tests being slow is a fair point, and I think this is something we can change: when writing these, speed was not one of the things I had in mind.

@benbovy
Copy link
Member

benbovy commented Nov 13, 2023

the issue with opt-in is that they might not run at all, which means that any bugs they would have caught will slip by unnoticed

Yes they should be run at least once! Are they run in all, many or just a few items of the CI matrix? Would it be possible to opt-in for just one or a few matrix items if that's not the case already?

@keewis
Copy link
Collaborator

keewis commented Nov 13, 2023

so far, every environment other than the min-deps contains pint, so that means almost every CI

@max-sixty
Copy link
Collaborator Author

Would it be possible to opt-in for just one or a few matrix items if that's not the case already?

We could have one environment that runs --run-nightly, either opt-in, or limited to a single env, or only on merges (so we pick up corner-case failures quickly and can revert, without slowing down PR feedback)

@TomNicholas
Copy link
Contributor

so far, every environment other than the min-deps contains pint, so that means almost every CI

This seems like overkill, couldn't we just remove pint from a few of the envs? Depends which tests you want to run more quicky @benbovy ?

@dcherian dcherian changed the title Pint tests are failing Restrict pint test runs Nov 13, 2023
@keewis keewis linked a pull request Feb 26, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tools
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants