8000 `yield_namespace_device_dtype_combinations` parametrization ID confusing for 'device1' · Issue #31042 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

yield_namespace_device_dtype_combinations parametrization ID confusing for 'device1' #31042

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
Closed
lucyleeow opened this issue Mar 21, 2025 · 4 comments · Fixed by #31074
Closed

yield_namespace_device_dtype_combinations parametrization ID confusing for 'device1' #31042

lucyleeow opened this issue Mar 21, 2025 · 4 comments · Fixed by #31074

Comments

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

Noticed in #29431 (comment)

In yield_namespace_device_dtype_combinations, for array_api_strict we yield CPU_DEVICE first, then device1. This results in the confusing pytest parametrization IDs:

  • array_api_strict-device2-float32 - where 'device2' actually refers to Device("device1") and not Device("device2")
  • array_api_strict-device1-float64 - where 'device1' actually refers to Device("CPU_DEVICE") and not Device("device1")

Not sure if there is an good fix for this / or if a fix is needed.

We could yield Device("device1") first but then the id "device2" would refer to Device("CPU_DEVICE") and not Device("device2")

The only way to completely avoid confusion would be to write a function that takes a params and amends the names e.g.,

@pytest.mark.parametrize(
    "namespace,device,dtype",
    list(yield_namespace_device_dtype_combinations()),
    ids=_get_namespace_device_dtype_test_ids
)

The BUT I am not sure if this is an over complicated solution to a trivial problem...

(Edit: We could just make a note in the yield_namespace_device_dtype_combinations docstring - this seems like a reasonable solution...)
@github-actions github-actions bot added the Needs Triage Issue requires triage label Mar 21, 2025
@lucyleeow
Copy link
Member Author

Actually I've just realised that the ID 'device' depends on the number of namespaces supported in the environment, but the above does seem to be the case for a few of the azure workflows.

@betatim
Copy link
Member
betatim commented Mar 21, 2025

This also tripped me up a while back. So it is probably worth fixing.

We use the "generate IDs" approach for other tests and it is really nice, so maybe we should just do that here as well. The other approach that could be nice, but not sure if it is feasible, is to modify array-api-strict's Device class so that it "automatically" gets represented nicer in tests. That would solve the problem for all people who use array-api-strict which would be nice.

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

is to modify array-api-strict's Device class so that it "automatically" gets represented nicer in tests.

I don't think this is possible. If it's not Numbers, strings, booleans and None, then pytest will use the argument name:

https://docs.pytest.org/en/stable/example/parametrize.html#different-options-for-test-ids

I was naively hoping a __str__ or __repr__ would do it but no. It actually may be a nice pytest feature to add.

Happy to add a 'generate ID' function, if others don't think its adding too much complexity

@ogrisel ogrisel mentioned this issue Mar 21, 2025
Merged
3 tasks
@ogrisel ogrisel removed the Needs Triage Issue requires triage label Mar 21, 2025
@ogrisel
Copy link
Member
ogrisel commented Mar 21, 2025

Happy to add a 'generate ID' function, if others don't think its adding too much complexity

I don't have a better solution to propose... and the current status quo is really misleading, so probably worth fixing.

@lucyleeow lucyleeow mentioned this issue Mar 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ogrisel @betatim @lucyleeow
0