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

[test] Add SYCL canary test to identify bad environment / configuration #1824

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

danhoeflinger
Copy link
Contributor

This PR adds a SYCL canary test, which is meant to be divorced for oneDPL specifics, and launch a minimal sycl kernel.

This is meant to be used in CI runs, to identify for dpcpp backends when the SYCL environment / setup is invalid, and stop before continuing to main oneDPL tests.

This should provide more stability to aggregated test pass statistics for SYCL, especially on windows, more robust to environmental issues, and provide a tool to more concretely separate issues in oneDPL vs issues in setup.

Copy link
Contributor

@timmiesmith timmiesmith left a comment

Choose a reason for hiding this comment

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

The test looks good to me.

@danhoeflinger
Copy link
Contributor Author

Do we want to name this something other than *.pass so it doesn't get included in our full test glob?

The internal CI specifically builds and runs this test separately, but there could be some advantage to have it included in the full build suite for runs outside the CI, so we can use it as a status there too.

timmiesmith
timmiesmith previously approved these changes Sep 6, 2024
@timmiesmith
Copy link
Contributor

Do we want to name this something other than *.pass so it doesn't get included in our full test glob?

The internal CI specifically builds and runs this test separately, but there could be some advantage to have it included in the full build suite for runs outside the CI, so we can use it as a status there too.

I agree. Having it compiled/run a second time isn't an issue and the naming should be consistent with other tests we expect to pass.

@danhoeflinger
Copy link
Contributor Author

danhoeflinger commented Sep 6, 2024

I'm going to hold on this PR until we have a system solid within the CI to handle it, in case other changes are found to be needed.

@danhoeflinger danhoeflinger changed the title Add SYCL canary test to identify bad environment / configuration [test] Add SYCL canary test to identify bad environment / configuration Sep 6, 2024
test()
{
sycl::queue q(default_selector);
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nitpick, but what is the reason for introducing a new scope for the call to submit() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there isn't one. I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@dmitriy-sobolev
Copy link
Contributor

dmitriy-sobolev commented Sep 6, 2024

@danhoeflinger, have you also considered replacing/complementing CMake checks of a compiler? It may be a more nature way to follow, compared to adding a new test along with other test. You can then add logic not to start testing if these checks do not pass, and this will not affect oneDPL pass rate.

You can try relying on https://cmake.org/cmake/help/v3.14/command/try_run.html, for example.

@danhoeflinger
Copy link
Contributor Author

@danhoeflinger, have you also considered replacing/complementing CMake checks of a compiler? It may be a more nature way to follow, compared to adding a new test along with other test. You can then add logic not to start testing if these checks do not pass, and this will not affect oneDPL pass rate.

You can try relying on https://cmake.org/cmake/help/v3.14/command/try_run.html, for example.

Yes, This was the first avenue I explored but decided against it for the following reasons:

  1. try_run / try_compile is not good with adding oneDPL as a link library and incorporating all the compiler flags etc. that come with that. I saw in the forums for CMake that because the config isn't complete at the time of the try_run command, it isn't able to use it as a link library and inherit the flags, etc. We also have specific environment variables we set for our tests. Therefore, it may be challenging and error prone to get it it in the same situation environment-wise with the rest of our tests, having to mirror every change with a matching one for the try_run command.
  2. try_run doesnt work so well with cross compilation

With this setup, we can very easily get the test in the same situation as our other tests from an environment + flag perspective.
I agree, I would prefer not to add this test to the pass rate as a passing test for each system, but that's relatively minor in the scheme of things. We also could work out the command to exclude this test explicitly from the main run. We need to make sure that plays nice with the filter commands passed in via the CI run fields though. I will look into that.

Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger
<[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger danhoeflinger force-pushed the dev/dhoeflin/canary_cmake_test_dpcpp branch from 070e38e to b32ba8a Compare September 10, 2024 20:02
{
#if TEST_DPCPP_BACKEND_PRESENT
# if _MSC_VER
char* pValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

This test uses a mix of camelCase, snake_case and double-underscore-prefixed snake_case. Maybe we should align on a single style for variable names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this test is a bit sloppy in that regard. I've fixed it up.

Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

5 participants