10000 MNT Add function to generate pytest IDs for `yield_namespace_device_dtype_combinations` by lucyleeow · Pull Request #31074 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT Add function to generate pytest IDs for yield_namespace_device_dtype_combinations #31074

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 30, 2025
Merged

MNT Add function to generate pytest IDs for yield_namespace_device_dtype_combinations #31074

merged 5 commits into from
Mar 30, 2025

Conversation

lucyleeow
Copy link
Member
@lucyleeow lucyleeow commented Mar 26, 2025 •
edited
Loading

Reference Issues/PRs

closes #31042

What does this implement/fix? Explain your changes.

Adds a function that generates IDs pytest parametrization IDs for yield_namespace_device_dtype_combinations.

The aim was to make it look the same as:

scikit-learn/sklearn/utils/_array_api.py

Lines 102 to 103 in 734245a

yield array_namespace, "CPU_DEVICE", "float64"
yield array_namespace, "device1", "float32"

i.e.

sklearn/metrics/cluster/tests/test_supervised.py::test_entropy_array_api[array_api_strict-CPU_DEVICE-float64] PASSED
sklearn/metrics/cluster/tests/test_supervised.py::test_entropy_array_api[array_api_strict-device1-float32] PASSED

I came up with the options:

  • (implemented) - check if the object is an array_api_strict.Device - this is least brittle but does require that we try to import array_api_strict when collecting tests
  • (commented out) - string manipulation - this is brittle because it replies on the array-api-strict.Device __repr__ staying same
    • I could instead use a regex, which would make it less brittle (i.e. .*\('([\w_]+)'\) - to capture what is in brackets)

Alternatively I could just use return f"{param}" but the ID would be long and slightly confusing difficult to read:

sklearn/metrics/cluster/tests/test_supervised.py::test_entropy_array_api[array_api_strict-array_api_strict.Device('CPU_DEVICE')-float64] PASSED
sklearn/metrics/cluster/tests/test_supervised.py::test_entropy_array_api[array_api_strict-array_api_strict.Device('device1')-float32] PASSED

Any other comments?

cc @ogrisel @betatim

@github-actions GitHub Actions
Copy link
github-actions bot commented Mar 26, 2025 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 03a2569. Link to the linter CI: here

lesteve
lesteve reviewed Mar 26, 2025
View reviewed changes
Copy link
Member
@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Do you think we should use the ids function in all the parametrized tests that use yield_namespace_device_dtype_combinations?

ogrisel
ogrisel approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. There are probably many existing parametrized test that deserve to be updated. Feel free to do it as part of this PR (or not).

@lucyleeow
Copy link
Member Author

There are probably many existing parametrized test that deserve to be updated

Yes I intend to do them in this PR, I was just not 100% on the implementation.

Are we happy with the current implementation, even though it tries to import array-api-strict during test collection?

Summary of possible options:

  1. import array-api-strict
  2. string manipulation - no need to import but brittle
  3. use repr of Device - no need to import but long ID

@lucyleeow
Copy link
Member Author

Thanks @ogrisel @lesteve

Changes made and I've added ids to all tests that use yield_namespace_device_dtype_combinations. I've kept the implementation that trys to imports array-api-strict.

I think there is nothing required in test_estimator_checks.py as the IDs don't get that detailed?

@lesteve
Copy link
Member
lesteve commented Mar 28, 2025

It seems to work fine locally when running:

pytest sklearn/utils/tests/test_array_api.py -v

You see that the tests are explicit about CPU_DEVICE vs device1 (part of the output below):

sklearn/utils/tests/test_array_api.py::test_count_nonzero[float--2-csr_array-array_api_strict-CPU_DEVICE-float64] PASSED                                                                                                                 [ 97%]
sklearn/utils/tests/test_array_api.py::test_count_nonzero[float--2-csr_array-array_api_strict-device1-float32] PASSED

lesteve
lesteve approved these changes Mar 28, 2025
View reviewed changes
sklearn/utils/_array_api.py
try:
import array_api_strict
except ImportError:
return None
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention the pytest doc link and mention that returning None doesn't change the default id if array-api-strict is not installed

Suggested change
return None
return

Maybe also quickly mention the fact that this gives better ids for array-api-strict devices?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mentioned that None means default but didn't add the link as it was long(er than char limit) and wasn't sure if it is needed, but happy to add if you wish.

@lesteve lesteve enabled auto-merge (squash) March 28, 2025 05:14
@lesteve lesteve changed the title Add function to generate pytest IDs for yield_namespace_device_dtype_combinations MNT Add function to generate pytest IDs for yield_namespace_device_dtype_combinations Mar 28, 2025
@lesteve
Copy link
Member
lesteve commented Mar 28, 2025

Weird Debian32 CI failure build log is similar to the one seen in main #29802 (comment), it seems all the PRs with a recent push are currently broken ...

@lesteve lesteve mentioned this pull request Mar 28, 2025
Open
@lesteve lesteve merged commit 6778690 into scikit-learn:main Mar 30, 2025
34 checks passed
@lucyleeow lucyleeow deleted the pytest_id_arrayapi branch March 30, 2025 06:41
lucyleeow added a commit to lucyleeow/scikit-learn that referenced this pull request Apr 2, 2025
@lucyleeow
MNT Add function to generate pytest IDs for `yield_namespace_device_d…
5b15faf 
…type_combinations` (scikit-learn#31074)
@lucyleeow lucyleeow mentioned this pull request Apr 11, 2025
Merged
lucyleeow added a commit to lucyleeow/scikit-learn that referenced this pull request Apr 14, 2025
@lucyleeow
Revert "MNT Add function to generate pytest IDs for `yield_namespace_…
7420997 
…device_dtype_combinations` (scikit-learn#31074)"

This reverts commit 6778690.
@lucyleeow lucyleeow mentioned this pull request Apr 14, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@lesteve lesteve lesteve approved these changes

Assignees
No one assigned
Labels
module:metrics module:utils No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

yield_namespace_device_dtype_combinations parametrization ID confusing for 'device1'
3 participants
@lucyleeow @lesteve @ogrisel
0