Skip to content

[UR] Move conformance tests over to lit #17998

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

Merged
merged 1 commit into from
May 7, 2025
Merged

Conversation

RossBrunton
Copy link
Contributor

@RossBrunton RossBrunton commented Apr 14, 2025

Conformance tests are now ran by LLVM's lit framework rather than ctest.
A new check-unified-runtime-conformance target is available, which
builds and runs all conformance tests. The test target will still run
them.

Some additional housekeeping was done:

  • add_conformance_test_with_x_environment was renamed to
    add_conformance_x_test.
  • Conformance tests now explicitly respect ONEAPI_DEVICE_SELECTOR.
  • If unspecified, ONEAPI_DEVICE_SELECTOR will now be populated with
    the list of built targets. Notably, this will exclude adapters which
    exist in the build directory but were not specified using a
    BUILD_ADAPTER cmake option (e.g, from a previous build before a
    reconfigure).
  • References to UR_CTS_ADAPTER_PLATFORM and UR_CTS_BACKEND are
    removed (they never did anything anyway).
  • Github build jobs have been updated.
  • The generated gtest binaries have been moved from build/bin to
    build/test/conformance
  • requirements_testing.txt now imports psutil for using timeouts.
  • Output format has been changed to show a progress bar and print
    the test output in real time.
  • Testing now respects the "device" filters from
    ONEAPI_DEVICE_SELECTOR (previously, the filter was only used to
    select the platform. opencl:cpu now works as expected.
  • Some CUDA tests are marked as failing since they are failing
    after this change.

As a resut of these changes, a build with both the v1 and v2
versions of the level zero adapter will only run the v2 adapter.
Building with only one version of the level zero adapter (as the
Github CI currently does) works as expected and only runs that
version.

Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

Please format the python to 80 columns and consider my final round of suggestions.

@RossBrunton
Copy link
Contributor Author

Please format the python to 80 columns and consider my final round of suggestions.

Benie can correct me if I'm wrong, but I don't think we have an established column limit for Python code? Black has a column limit of 88, but sometimes goes over it, especially with comments and strings.

@aelovikov-intel
Copy link
Contributor

Please format the python to 80 columns and consider my final round of suggestions.

Benie can correct me if I'm wrong, but I don't think we have an established column limit for Python code? Black has a column limit of 88, but sometimes goes over it, especially with comments and strings.

IMO, if formatter doesn't complain then formatting is OK.

@ldrumm
Copy link
Contributor

ldrumm commented May 1, 2025

Black has a column limit of 88, but sometimes goes over it, especially with comments and strings.

I hate this - especially as automated formatting is meant to give consistency; the rest of llvm has a strict 80 column limit which clang-format works extremely hard to maintain; and PEP-8 says 79(!). But since upstream LLVM have somehow decided darker is the formatter of choice, I concede.

@ldrumm
Copy link
Contributor

ldrumm commented May 1, 2025

Just found this: https://discourse.llvm.org/t/rfc-document-and-standardize-python-code-style/68257/33

I'll take my grievances to discourse

Conformance tests are now ran by LLVM's lit framework rather than ctest.
A new `check-unified-runtime-conformance` target is available, which
builds and runs all conformance tests. The `test` target will still run
them.

Some additional housekeeping was done:
* `add_conformance_test_with_x_environment` was renamed to
  `add_conformance_x_test`.
* Conformance tests now explicitly respect `ONEAPI_DEVICE_SELECTOR`.
* If unspecified, `ONEAPI_DEVICE_SELECTOR` will now be populated with
  the list of built targets. Notably, this will exclude adapters which
  exist in the build directory but were not specified using a
  `BUILD_ADAPTER` cmake option (e.g, from a previous build before a
  reconfigure).
* References to `UR_CTS_ADAPTER_PLATFORM` and `UR_CTS_BACKEND` are
  removed (they never did anything anyway).
* Github build jobs have been updated.
* The generated gtest binaries have been moved from `build/bin` to
  `build/test/conformance`
* `requirements_testing.txt` now imports psutil for using timeouts.
* Output format has been changed to show a progress bar and print
  the test output in real time.
* Testing now respects the "device" filters from
  `ONEAPI_DEVICE_SELECTOR` (previously, the filter was only used to
  select the platform. `opencl:cpu` now works as expected.
* Some CUDA tests are marked as failing since they are failing
  after this change.

As a resut of these changes, a build with both the v1 and v2
versions of the level zero adapter will only run the v2 adapter.
Building with only one version of the level zero adapter (as the
Github CI currently does) works as expected and only runs that
version.
@RossBrunton
Copy link
Contributor Author

@intel/dpcpp-devops-reviewers Just need a quick look at the workflow files - it's just a tweak to run the conformance testing in the new way.

@@ -184,7 +171,8 @@ jobs:
- name: Test adapters
env:
ZE_ENABLE_LOADER_DEBUG_TRACE: 1
run: env UR_CTS_ADAPTER_PLATFORM="${{matrix.adapter.platform}}" ctest -C ${{matrix.build_type}} --test-dir build --output-on-failure -L "conformance" --timeout 600 -VV
LIT_OPTS: " --timeout 120"
run: cmake --build build -j $(nproc) -- check-unified-runtime-conformance
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you use ninja you don't need to set -j

@RossBrunton
Copy link
Contributor Author

@intel/llvm-gatekeepers I think this should be ready to merge.

@sarnex We don't use Ninja, I'm afraid.

@igchor With the "building V1 and V2 at the same time" thing, I know this is a change in behaviour, but since it doesn't affect our CI I think it makes sense to merge this as is. If running both v1 and v2 in the same build is desired, I can create a followup MR to add a new cmake target to allow that.

@kbenzie kbenzie merged commit 885c439 into intel:sycl May 7, 2025
21 checks passed
kbenzie added a commit that referenced this pull request May 9, 2025
sarnex pushed a commit that referenced this pull request May 9, 2025
github-actions bot pushed a commit to oneapi-src/unified-runtime that referenced this pull request May 10, 2025
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.

7 participants