-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat: numpy 2.0rc support #651
Conversation
uv pip install ${{ matrix.pre-release-dependencies }} -e ".[test-all]" --system | ||
uv pip freeze | ||
- name: Test with pytest | ||
run: pytest -n auto --disable-warnings --cov=sklego -m "cvxpy or formulaic or umap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder ... is the -m
part needed here? Don't we test for everything by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh true, by default everything is tested, with such flag just the extras. I was thinking doing a sort of "delta".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can still be a good idea. I guess we could first do a run without those marked tests and then run a step for each of them. Just make sure that you always run each step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split each extra using matrix, it gets a little spammy but very detailed (example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall, just wanted to double-check two things.
uv pip freeze | ||
- name: Test with pytest | ||
run: pytest -n auto --disable-warnings --cov=sklego -m "not cvxpy and not formulaic and not umap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, there is no easy way to ask only for those tests without a "registered" mark.
I tried to achieve that dynamically via pytest_collection_modifyitems, but it's a bit of a headache as @pytest.mark.parametrize
and any other pytest marker count as a mark in the items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As-is seems perfectly fine :)
Description
There are 3 folds on this PR:
np.lib.pad
tonp.pad
np.NaN
tonp.nan
numpy<2.0
for cvxpy and umap extra dependencies: it is out of our control for them to support latest numpy version.Edit: Manual trigger of cron gha is successful