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

Add test phase in CI for stdexec configuration #930

Merged
merged 6 commits into from
Sep 13, 2023

Conversation

aurianer
Copy link
Collaborator

@aurianer aurianer commented Jul 6, 2023

@aurianer aurianer self-assigned this Jul 6, 2023
@rasolca
Copy link
Collaborator

rasolca commented Jul 6, 2023

cscs-ci run

@aurianer aurianer force-pushed the enable_stdexec_test branch from 7c4321b to 8ec8a1c Compare August 3, 2023 13:40
@aurianer
Copy link
Collaborator Author

aurianer commented Aug 3, 2023

Note: I have a timeout on test_tridiag_solver_distributed, investigating..
EDIT: The hang has been introduced by removing the ensure_started in applyDeflation and calcTolerance. I will look at removing those in a separate PR (issue #954 opened).

@aurianer aurianer force-pushed the enable_stdexec_test branch from 8ec8a1c to 3e43875 Compare August 3, 2023 13:53
@aurianer aurianer marked this pull request as ready for review August 11, 2023 17:41
@aurianer aurianer force-pushed the enable_stdexec_test branch 3 times, most recently from 97e3d63 to 2e6a577 Compare August 11, 2023 17:46
@aurianer
Copy link
Collaborator Author

cscs-ci run

Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

Thanks @aurianer for the investigation here.

Functionally I'm happy with this change but see inline comment before I'm ready to approve this.

Linking #665 as well since it's a related issue. I think the solution here is necessary, but possibly not sufficient for full control of tile lifetimes (as the remaining ensure_started problem hints at).

@aurianer aurianer force-pushed the enable_stdexec_test branch from 2e6a577 to 088c560 Compare August 14, 2023 17:20
@aurianer aurianer force-pushed the enable_stdexec_test branch from 088c560 to 90bda4a Compare August 15, 2023 11:25
@aurianer
Copy link
Collaborator Author

cscs-ci run

Copy link
Collaborator

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

This looks good to me now, thanks!

I'd still like to wait to see the benchmark results, but assuming there are no surprises there this requires no further changes.

The missing includes from unwrap.h are not a blocker for this PR.

@aurianer aurianer force-pushed the enable_stdexec_test branch from 90bda4a to 9b8da64 Compare August 24, 2023 15:35
@aurianer
Copy link
Collaborator Author

So I:

  • added the missing include
  • change the early release implementation with @msimberg suggestion which fixes the performance regression
  • just launched the benchmark with the new version

TODO: Post the benchmark results when available

@aurianer aurianer force-pushed the enable_stdexec_test branch from 9b8da64 to 975e6d8 Compare August 25, 2023 12:59
@msimberg
Copy link
Collaborator

cscs-ci run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants