-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL] Remove flaky test #9063
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
[SYCL] Remove flaky test #9063
Conversation
See intel#9008 Signed-off-by: Sarnie, Nick <[email protected]>
I don't think this test is failing anymore. |
Co-authored-by: Alexey Bader <[email protected]>
@bso-intel 2 of the 3 commits today have this problem: https://github.com/intel/llvm/actions/runs/4689271741/jobs/8311985618 |
@bader applied your feedback, we can prolly merge without ci (maybe?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the failure links.
I think this test is inherently flaky.
Just comparing memory usage before and after the SYCL kernel is not a good method to check the memory leak.
We don't know whether there is other thread / task is running in addition to the kernel.
So, this test is not very useful.
@bso-intel thanks, if i made another PR after this to just delete the test would you support it? |
Yes, I would like to remove this test rather than disable it. |
ok ill do that next |
@@ -7,6 +7,8 @@ | |||
// extra memory allocation on device creation. | |||
// Issue #661: The test is failing sporadically on HIP AMD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this test has been already identified "flaky" although it was on HIP AMD platform before.
Anyway, measuring memory consumption at two different times cannot check memory leak reliably.
Let's remove this test from our E2E test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in latest commit
Signed-off-by: Sarnie, Nick <[email protected]>
@intel/llvm-gatekeepers Hey all can we merge? The gen12 failure is not related and this fixes a CI problem hit in every PR and every postcommit run. Thx. |
Closes: #9008