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 unit test cases and fixes for the S/R versions of the parallel algorithms #6494

Merged
merged 48 commits into from
Sep 3, 2024

Conversation

zhekemist
Copy link
Contributor

@zhekemist zhekemist commented May 22, 2024

This PR has the goal of adding simple unit test cases for the S/R versions of HPX's parallel algorithms where they were previously missing, and to address the issues identified through these test. The fixes are currently based on NVIDIA's stdexec S/R implementation instead of HPX's own S/R implementation. (see PR #6431)

The algorithms addressed in this PR are tracked in this document. Additionally, Issue #6535 was opened to track the algorithms not covered in this PR.

Note that the S/R unit tests for algorithms not yet fixed were removed to avoid cluttering the tests with unnecessary, guaranteed-to-fail test cases. They were removed in commit a20f1b4 and can be restored by reverting this commit if needed.

This commit introduces simple unit test cases for the S/R (Sender
and Receiver) versions of the parallel algorithms where they did not
exist yet. Note that this is only the first part of these additional
test cases, they have not been implemented for every algorithm yet.
@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link

codacy-production bot commented May 22, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-4.60% 9.87%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (bd43586) 228282 194725 85.30%
Head commit (e35074d) 207698 (-20584) 167618 (-27107) 80.70% (-4.60%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#6494) 4790 473 9.87%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

All usages of the tt::sync_wait sender consumer in the recently
added unit tests were replaced with equivalent function calls
in order to make the tests compatible with NVIDIA's stdexec.
This commit adds the remaining sender adaptor test cases. As of now,
it is not planned to add further test cases, as test cases were
added to all the algorithms where they are currently of interest.
@hkaiser
Copy link
Member

hkaiser commented May 28, 2024

Wow... Just wow! Thanks a lot for your work on this!

zhekemist and others added 7 commits June 28, 2024 10:21
This commit introduces a first batch of fixes for the
S/R versions of some parallel algorithms. However,
they are currently all build on a temporary solution
for an issue in the "then sender" and therefore not
final yet.
- Fly-by: comment out sender tests for algorithms that are not fixed yet
zhekemist and others added 9 commits July 29, 2024 15:05
The guard prevents the use of invocables returning bool in the
bulk_async_execute method of the explicit_scheduler_executor to
avoid data races. Using such invocables can cause issues because
std::vector<return type of the invocable> may be simultaneously
written to by multiple threads, which is problematic
with std::vector<bool> due to its space optimizations.
This commit introduces a new set of fixes for the
S/R versions of several more parallel algorithms and
reworks of some previous, buggy fixes. Additionally,
new unit test cases have been added for some of the
algorithms to cover edge cases such as empty ranges in
case they could become pitfalls for the S/R adaptations.
This led to the discovery of some general bugs in the
algorithms, which were fixed as flybys:
* `first_first_of` (+ regression test case)
* `ends_with`
Fly-by: Rebalance the two target lists
Flybys: add missing newline at EOF, fix typo
`stellargroup/build_env:14` uses Clang 10, which lacks support for
`std::identity`, a type explicitly required for enabling stdexec in
HPX. Since `stellargroup/build_env:16` uses Clang 12, switching
to it should resolve this issue.
Fly-by: Reenable S/R unit test for rotate
Remove the S/R unit tests of the algorithms that are not fixed yet,
since they will fail anyways and currently only pollute the test
folder. To resume work on these tests in the future, simply revert
this commit.
The unit tests for the S/R versions of the parallel algorithms are only configured
if HPX_WITH_STDEXEC is enabled. Additionally, the tests cases that are in headers
shared with the non-S/R tests are surrounded with macro guards checking if stdexec
is available.
* Add missing headers
* Update copyright comments
* Make formatting and naming more consistent
zhekemist and others added 8 commits August 19, 2024 10:52
@zhekemist zhekemist changed the title Add unit test cases for the S/R versions of the parallel algorithms Add unit test cases and fixes for the S/R versions of the parallel algorithms Aug 26, 2024
`transfer_just` was removed in the 10th revision of P2300, this
commit replaces its usage in the `explicit_scheduler_executor`.
@zhekemist zhekemist marked this pull request as ready for review August 28, 2024 14:28
@isidorostsa
Copy link
Contributor

@hkaiser could you enable testing?

@isidorostsa
Copy link
Contributor

@zhekemist Should this be merged as is?

@hkaiser
Copy link
Member

hkaiser commented Sep 3, 2024

Yes, we can merge this. The CI problems are unrelated.

@hkaiser hkaiser merged commit f4ff6e8 into STEllAR-GROUP:master Sep 3, 2024
69 of 80 checks passed
@hkaiser
Copy link
Member

hkaiser commented Sep 3, 2024

Thanks a lot @zhekemist!

@isidorostsa
Copy link
Contributor

Great work Tobias, thank you for this thorough and well documented contribution!

@zhekemist zhekemist deleted the add-sr-unit-tests branch September 30, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants