Skip to content

[SYCL] Adapt to sycl 2020 exceptions #9771

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 27, 2023
Merged

[SYCL] Adapt to sycl 2020 exceptions #9771

merged 6 commits into from
Jun 27, 2023

Conversation

pwisniewskimobica
Copy link
Contributor

Due to issue #7268
I changed old exceptions to new ones (sycl::exception)

@pwisniewskimobica pwisniewskimobica requested a review from a team as a code owner June 7, 2023 13:55
@pwisniewskimobica pwisniewskimobica mentioned this pull request Jun 7, 2023
Closed
steffenlarsen
steffenlarsen reviewed Jun 7, 2023
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Thanks a lot for making these changes! I just have a small request and then it needs to be formatted with clang-format, then I'm happy with it. 😄

@steffenlarsen steffenlarsen changed the title adapt to sycl 2020 exceptions [SYCL] Adapt to sycl 2020 exceptions Jun 7, 2023
@pwisniewskimobica
Copy link
Contributor Author

@steffenlarsen Thanks for comments.
Now I formatted it with clang-format tool and also made that explicitly conversion you mentioned.
Please take a look.
Regards

@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 20, 2023 16:23 — with GitHub Actions Inactive
steffenlarsen
steffenlarsen approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Great, thank you! There seems to be some failing tests due to them expecting the old exceptions, but otherwise it LGTM!

@pwisniewskimobica
Copy link
Contributor Author

@steffenlarsen thanks
Do you think I also need to fix those tests? Or leave at it is at let you or someone to merge this PR?

@steffenlarsen
Copy link
Contributor

steffenlarsen commented Jun 21, 2023 •
edited
Loading

@steffenlarsen thanks Do you think I also need to fix those tests? Or leave at it is at let you or someone to merge this PR?

We need the tests to pass before merging the PR, otherwise it will make other PRs and the post-commit testing fail these tests too.

Note that the unittests that fail seem to all be in sycl/unittests/allowlist/ParseAllowList.cpp, trying to catch sycl::runtime_error specifically. Changing those to expect sycl::exception should do the trick. That said, there could still be other tests that fail when these are fixed, as E2E tests haven't been run due to the build-job-related tests failing.

@pwisniewskimobica pwisniewskimobica force-pushed the adapt_code_to_sycl_2020_exceptions branch 4 times, most recently from b820f76 to 2a42cf8 Compare June 23, 2023 10:44
@pwisniewskimobica pwisniewskimobica force-pushed the adapt_code_to_sycl_2020_exceptions branch from a3a869e to 95ed228 Compare June 23, 2023 10:59
@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 23, 2023 11:03 — with GitHub Actions Inactive
@pwisniewskimobica pwisniewskimobica force-pushed the adapt_code_to_sycl_2020_exceptions branch from 95ed228 to 88d14f0 Compare June 23, 2023 11:08
@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 23, 2023 11:18 — with GitHub Actions Inactive
@pwisniewskimobica pwisniewskimobica force-pushed the adapt_code_to_sycl_2020_exceptions branch 2 times, most recently from 29f1762 to a410e75 Compare June 23, 2023 11:37
@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 23, 2023 11:53 — with GitHub Actions Inactive
@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 23, 2023 12:32 — with GitHub Actions Inactive
@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 23, 2023 14:00 — with GitHub Actions Inactive
@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 23, 2023 14:58 — with GitHub Actions Inactive
@pwisniewskimobica
Copy link
Contributor Author

@steffenlarsen Thanks for comment
I fixed tests related to my change and also included them in separate commit in PR.
Unfortunately as you wrote there are some old e2e tests failing which are not trivial for me to fix as they are not related.
Please take a look
thanks

@steffenlarsen
Copy link
Contributor

@steffenlarsen Thanks for comment I fixed tests related to my change and also included them in separate commit in PR. Unfortunately as you wrote there are some old e2e tests failing which are not trivial for me to fix as they are not related. Please take a look thanks

Thank you, @pwisniewskimobica ! At first glance I don't think any of these failures are related to this patch. Normally it may help to push a merge with tip onto the this patch and let CI run again, though we have been seeing some odd failures in CI today so I cannot promise that it will fix it fully.

@pwisniewskimobica
Copy link
Contributor Author

@steffenlarsen thanks
What you propose for me about this PR?

@steffenlarsen
Copy link
Contributor

With the amount of failures in the current version I would prefer if you push a merge commit and hopefully it will at least reduce the number of failures we see. My worry is that if we consider the current failures as unrelated we may miss some that are in fact related.

@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 27, 2023 09:22 — with GitHub Actions Inactive
@pwisniewskimobica pwisniewskimobica temporarily deployed to aws June 27, 2023 10:31 — with GitHub Actions Inactive
@pwisniewskimobica
Copy link
Contributor Author

@steffenlarsen thanks for help!
Its seems like merging resolved that errors:)
You you take a look and merge if possible?
Regards

@steffenlarsen
Copy link
Contributor

So it did! Thanks again for making these changes.

@steffenlarsen steffenlarsen merged commit f08dc94 into intel:sycl Jun 27, 2023
Chenyang-L pushed a commit that referenced this pull request Jul 10, 2023
@pwisniewskimobica @Chenyang-L
[SYCL] Adapt to sycl 2020 exceptions (#9771)
ed9e4aa 
Due to issue #7268
I changed old exceptions to new ones (`sycl::exception`)
@Chenyang-L Chenyang-L mentioned this pull request Jul 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steffenlarsen steffenlarsen steffenlarsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pwisniewskimobica @steffenlarsen