Skip to content

[SYCL][Test E2E] Change 'CHECK-NEXT' to 'CHECK' of abort_internalization.cpp #9834

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 6 commits into from
Sep 12, 2023

Conversation

myler
Copy link
Contributor

@myler myler commented Jun 13, 2023

To make the test more robust with ZE_DEBUG environment.

@myler myler temporarily deployed to aws June 13, 2023 03:07 — with GitHub Actions Inactive
@myler myler temporarily deployed to aws June 13, 2023 03:46 — with GitHub Actions Inactive
@myler
Copy link
Contributor Author

myler commented Jun 13, 2023

Unrelated failure:
TEST 'SYCL :: Basic/handler/handler_mem_op.cpp' FAILED

Copy link
Contributor

@victor-eds victor-eds left a comment

Choose a reason for hiding this comment

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

Can we get a rationale of this change and an actual description? I understand why the change is needed, but I'd argue that should be part of the description

@myler myler changed the title [SYCL][Test E2E] skip running abort_internalization.cpp with ZE_DEBUG environment. [SYCL][Test E2E] skip running abort_internalization.cpp with ZE_DEBUG environment because the output break continuous checking with "// CHECK-NEXT" directives. Jun 14, 2023
@myler myler temporarily deployed to aws June 14, 2023 01:58 — with GitHub Actions Inactive
@myler myler temporarily deployed to aws June 14, 2023 02:37 — with GitHub Actions Inactive
@myler myler requested a review from victor-eds June 14, 2023 03:11
@victor-eds
Copy link
Contributor

I'd move the "because the output break continuous checking with "// CHECK-NEXT" directives." part from the commit title to the body, similar to: "Do not run this test as L0 debug output breaks CHECK-NEXT directives in test"

@myler myler changed the title [SYCL][Test E2E] skip running abort_internalization.cpp with ZE_DEBUG environment because the output break continuous checking with "// CHECK-NEXT" directives. [SYCL][Test E2E] skip running abort_internalization.cpp with ZE_DEBUG environment Jun 15, 2023
@myler
Copy link
Contributor Author

myler commented Jun 19, 2023

@victor-eds please check again, I moved reason to description.

@jandres742
Copy link
Contributor

Do not run this test as L0 debug output breaks CHECK-NEXT directives in test.

shouldn't we try to fix the test or the way we are running, instead of not running it?

@myler
Copy link
Contributor Author

myler commented Jun 19, 2023

Do not run this test as L0 debug output breaks CHECK-NEXT directives in test.

shouldn't we try to fix the test or the way we are running, instead of not running it?

Hi @sommerlukas,
I checked the commit history, seems you are this test owner, could you help to check this issue? Is it possible to fix the test with ZE_DEBUG environment?

@Naghasan
Copy link
Contributor

I checked the commit history, seems you are this test owner, could you help to check this issue? Is it possible to fix the test with ZE_DEBUG environment?

We are no level 0 expert and we do not use it beyond what the CI requires us to do. We are happy to assist you in making the test more resilient, but when it comes to the actual test failures / testing environment, you have the data.

IIRC from similar failure with debug level 0, the issue is simply that you have a debug dump that is interleaved with the sycl log we are checking. In this test the CHECK-NEXT is probably overkill (it is checking for a sequence so it makes sense, but if that sequence goes out of sync the test will fail anyway), so I would be happy to merge a patch changing this to CHECK.

@victor-eds
Copy link
Contributor

Changing to CHECK LGTM. Better than not running the test.

@myler
Copy link
Contributor Author

myler commented Jun 25, 2023

I checked the commit history, seems you are this test owner, could you help to check this issue? Is it possible to fix the test with ZE_DEBUG environment?

We are no level 0 expert and we do not use it beyond what the CI requires us to do. We are happy to assist you in making the test more resilient, but when it comes to the actual test failures / testing environment, you have the data.

IIRC from similar failure with debug level 0, the issue is simply that you have a debug dump that is interleaved with the sycl log we are checking. In this test the CHECK-NEXT is probably overkill (it is checking for a sequence so it makes sense, but if that sequence goes out of sync the test will fail anyway), so I would be happy to merge a patch changing this to CHECK.

Have you done the change or do I need to make this change in current PR?

@sommerlukas
Copy link
Contributor

I checked the commit history, seems you are this test owner, could you help to check this issue? Is it possible to fix the test with ZE_DEBUG environment?

We are no level 0 expert and we do not use it beyond what the CI requires us to do. We are happy to assist you in making the test more resilient, but when it comes to the actual test failures / testing environment, you have the data.
IIRC from similar failure with debug level 0, the issue is simply that you have a debug dump that is interleaved with the sycl log we are checking. In this test the CHECK-NEXT is probably overkill (it is checking for a sequence so it makes sense, but if that sequence goes out of sync the test will fail anyway), so I would be happy to merge a patch changing this to CHECK.

Have you done the change or do I need to make this change in current PR?

We don't have an active PR to incorporate this change, so you can do it as part of this PR.

@myler myler temporarily deployed to aws June 26, 2023 08:10 — with GitHub Actions Inactive
@myler myler temporarily deployed to aws June 26, 2023 08:43 — with GitHub Actions Inactive
@myler myler temporarily deployed to aws June 27, 2023 05:58 — with GitHub Actions Inactive
@myler myler temporarily deployed to aws June 27, 2023 06:32 — with GitHub Actions Inactive
@myler
Copy link
Contributor Author

myler commented Jun 27, 2023

Unrelated failures:

Failed Tests (10):
  SYCL :: Basic/context-with-multiple-devices.cpp
  SYCL :: Basic/fpga_tests/fpga_aocx.cpp
  SYCL :: Basic/fpga_tests/fpga_dsp_control.cpp
  SYCL :: Basic/fpga_tests/fpga_latency_control_lsu.cpp
  SYCL :: Basic/fpga_tests/fpga_latency_control_pipe.cpp
  SYCL :: Basic/fpga_tests/fpga_lsu.cpp
  SYCL :: Basic/fpga_tests/global_fpga_device_selector.cpp
  SYCL :: DeviceLib/complex-fpga.cpp
  SYCL :: OnlineCompiler/online_compiler_OpenCL.cpp
  SYCL :: Scheduler/DISABLED_DataMovement.cpp

@myler myler temporarily deployed to aws July 21, 2023 03:56 — with GitHub Actions Inactive
@myler myler temporarily deployed to aws July 21, 2023 04:34 — with GitHub Actions Inactive
@myler
Copy link
Contributor Author

myler commented Jul 24, 2023

Ping @intel/llvm-gatekeepers this PR is ready to merge.

@sommerlukas
Copy link
Contributor

@myler Can you please update the PR description (which will become the commit message) to match the changes made in the PR?

@myler myler changed the title [SYCL][Test E2E] skip running abort_internalization.cpp with ZE_DEBUG environment [SYCL][Test E2E] change 'CHECK-NEXT' to 'CHECK' of abort_internalization.cpp to make the test more robust with ZE_DEBUG environment Jul 24, 2023
@myler
Copy link
Contributor Author

myler commented Jul 24, 2023

@myler Can you please update the PR description (which will become the commit message) to match the changes made in the PR?

Thanks for the reminder, I updated the title and removed unused description.

@myler
Copy link
Contributor Author

myler commented Jul 31, 2023

Ping @intel/llvm-gatekeepers this PR is ready to merge.

@bader bader changed the title [SYCL][Test E2E] change 'CHECK-NEXT' to 'CHECK' of abort_internalization.cpp to make the test more robust with ZE_DEBUG environment [SYCL][Test E2E] change 'CHECK-NEXT' to 'CHECK' of abort_internalization.cpp Sep 12, 2023
@bader bader changed the title [SYCL][Test E2E] change 'CHECK-NEXT' to 'CHECK' of abort_internalization.cpp [SYCL][Test E2E] Change 'CHECK-NEXT' to 'CHECK' of abort_internalization.cpp Sep 12, 2023
@bader bader merged commit ca56d79 into intel:sycl Sep 12, 2023
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.

6 participants