Skip to content

[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

Merged
merged 3 commits into from
Apr 13, 2023
Merged

[SYCL] Remove flaky test #9063

merged 3 commits into from
Apr 13, 2023

Conversation

sarnex
Copy link
Contributor

@sarnex sarnex commented Apr 13, 2023

Closes: #9008

See intel#9008

Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex temporarily deployed to aws April 13, 2023 16:31 — with GitHub Actions Inactive
@bso-intel
Copy link
Contributor

I don't think this test is failing anymore.
I tried locally and it is passing.
I also watched some PRs, and they seem passed.
For example, #9059

Co-authored-by: Alexey Bader <[email protected]>
@sarnex
Copy link
Contributor Author

sarnex commented Apr 13, 2023

@sarnex
Copy link
Contributor Author

sarnex commented Apr 13, 2023

@bader applied your feedback, we can prolly merge without ci (maybe?)

Copy link
Contributor

@bso-intel bso-intel left a 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.

@sarnex
Copy link
Contributor Author

sarnex commented Apr 13, 2023

@bso-intel thanks, if i made another PR after this to just delete the test would you support it?

@bso-intel
Copy link
Contributor

@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.
Unless we have a way to control OS from scheduling any other parallel task/thread running on the entire system, this test may pass today, it will come back and fail some day.

@sarnex
Copy link
Contributor Author

sarnex commented Apr 13, 2023

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.
Copy link
Contributor

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.

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 in latest commit

Signed-off-by: Sarnie, Nick <[email protected]>
@bso-intel bso-intel changed the title [SYCL] Disable flaky test [SYCL] Remove flaky test Apr 13, 2023
@sarnex sarnex temporarily deployed to aws April 13, 2023 18:52 — with GitHub Actions Inactive
@sarnex sarnex temporarily deployed to aws April 13, 2023 20:41 — with GitHub Actions Inactive
@sarnex
Copy link
Contributor Author

sarnex commented Apr 13, 2023

@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.

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.

Basic/memory-consumption.cpp fails on unrelated PRs
3 participants