Repository navigation
Add sycl_platform::has_aspect properties - #2412
ndgrigorian wants to merge 3 commits into
Conversation
|
View rendered docs @ https://intelpython.github.io/dpctl/pulls/2412/index.html |
|
coverage: 75.0% (+0.04%) from 74.963% — add-sycl-platform-info-queries into master |
There was a problem hiding this comment.
🟡 Changes recommended
The three new extension-aspect properties are missing from both Python and C++ test lists, leaving their property smoke checks and platform/device agreement checks uncovered.
1 open issue
What changed in this PR
This PR adds platform-wide aspect queries to dpctl.SyclPlatform, exposing capabilities supported by all devices on a platform.
Changes:
- Adds a C API aspect query and its Cython binding.
- Exposes 22
has_aspect_*properties with docstrings and refreshes the documentedlsplatformoutput. - Adds Python and C++ checks for standard aspects, platform/device agreement, and null-platform handling.
| File | Description |
|---|---|
| libsyclinterface/tests/test_sycl_platform_interface.cpp | Tests aspect queries against device capabilities and null platforms. |
| libsyclinterface/source/dpctl_sycl_platform_interface.cpp | Implements the platform aspect query with error handling. |
| libsyclinterface/include/syclinterface/dpctl_sycl_platform_interface.h | Declares and documents the new C API. |
| dpctl/tests/test_sycl_platform.py | Adds property checks and platform/device agreement tests. |
| dpctl/_sycl_platform.pyx | Adds aspect properties and updates output examples in documentation. |
| dpctl/_backend.pxd | Declares the C API function for Cython use. |
Review effort: Balanced
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Thanks, this looks good overall. A few points inline. The main one is that three of the new properties aren't covered by the tests. Could you also add a line to CHANGELOG.md under [dev] / Added for the new SyclPlatform.has_aspect_* properties?
8560194 to
efd37c7
Compare
Cover is_component, is_composite and ext_oneapi_ipc_memory in the Python and C-API platform aspect tests, note empty platforms in the docstrings, document the false-on-error return, and add a changelog entry.
efd37c7 to
99176e1
Compare
| list_of_platform_aspects = [ | ||
| "cpu", | ||
| "gpu", | ||
| "accelerator", | ||
| "custom", | ||
| "fp16", | ||
| "fp64", | ||
| "atomic64", | ||
| "image", | ||
| "online_compiler", | ||
| "online_linker", | ||
| "queue_profiling", | ||
| "usm_device_allocations", | ||
| "usm_host_allocations", | ||
| "usm_shared_allocations", | ||
| "usm_system_allocations", | ||
| "usm_atomic_host_allocations", | ||
| "usm_atomic_shared_allocations", | ||
| "host_debuggable", | ||
| "emulated", | ||
| "is_component", | ||
| "is_composite", | ||
| "ext_oneapi_ipc_memory", | ||
| ] |
There was a problem hiding this comment.
This list is hard-coded, so nothing checks that SyclPlatform has the same aspects as SyclDevice. #2416 adds six SyclDevice.has_aspect_ext_oneapi_* properties. With both merged, the platform would miss them and every test would still pass (the drift from #2412 (comment)). A test like this would catch it:
import dpctl
def test_platform_and_device_aspects_match():
def aspects(cls):
return {n for n in dir(cls) if n.startswith("has_aspect_")}
assert aspects(dpctl.SyclDevice) == aspects(dpctl.SyclPlatform)There was a problem hiding this comment.
Added test_platform_and_device_aspects_match in 65bda32, as suggested. It also checks list_of_platform_aspects against the properties, so a new aspect missing from the test list fails too.
Catch SyclPlatform.has_aspect_* drifting from SyclDevice, or from the hard-coded test list.
This PR proposes the addition of the
sycl_platform::has_aspectproperties todpctl.SyclPlatform