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

Fix incorrect tests in test_usm_ndarray_dlpack #1982

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ndgrigorian
Copy link
Collaborator

The test test_dlpack_device attempted to query a device using DLPack device ID from the list of root devices created by all_root_devices fixture in test_usm_ndarray_dlpack.

This logic is incorrect, as all_root_devices caches only the first two root devices per platform. As the DLPack device ID is based on the ordinal ID of the device as it is canonically presented in get_devices, this could fail if a given platform had more than 2 associated devices (in this case, for a machine with 2 PVC. Each has 2 stacks, and therefore, 4 level_zero and opencl:gpu devices).

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • Have you added documentation for your changes, if necessary?
  • Have you added your changes to the changelog?
  • If this PR is a work in progress, are you opening the PR as a draft?

These tests would fail on machines with more than 2 devices for a given platform due to an incorrect asusmption that the DLPack device ID would match that of the cached root devices, of which only 2 are kept per platform
Copy link

github-actions bot commented Feb 1, 2025

Copy link

github-actions bot commented Feb 1, 2025

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_503 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 88.24%. remained the same
when pulling 7ef0e44 on fix-dlpack-test
into bfd4d57 on master.

@ndgrigorian ndgrigorian marked this pull request as ready for review February 1, 2025 01:38
Copy link
Collaborator

@oleksandr-pavlyk oleksandr-pavlyk left a comment

Choose a reason for hiding this comment

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

Yes, you're right. Expect this change to slow down run-time of this test file on the machine with many cards, but correctness first.

Was it failing in Tiber?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants