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

Fix segmented algorithms tests #6221

Merged
merged 2 commits into from Apr 15, 2023

Conversation

Pansysk75
Copy link
Member

Fixes a group of tests for HPX segmented algorithms, which were occasionally failing due to improper initialization of partitioned_vector. Initialization using partitioned_vector::set_value() is asynchronous and returns a future, but we never waited for it to complete.

@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

@StellarBot
Copy link

Performance test report

HPX Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each=---(=)

Info

PropertyBeforeAfter
HPX Commit6b6e1e795f5b31
HPX Datetime2023-03-06T15:59:25+00:002023-04-12T13:40:36+00:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Datetime2023-03-10T03:27:49.135034-06:002023-04-12T08:50:34.167277-05:00
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch=

Info

PropertyBeforeAfter
HPX Commit6b6e1e795f5b31
HPX Datetime2023-03-06T15:59:25+00:002023-04-12T13:40:36+00:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Datetime2023-03-10T03:28:21.991297-06:002023-04-12T08:51:06.050512-05:00
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1

Comparison

BENCHMARKFORK_JOIN_EXECUTOR_DEFAULT_FORK_JOIN_POLICY_ALLOCATORPARALLEL_EXECUTOR_DEFAULT_PARALLEL_POLICY_ALLOCATORSCHEDULER_EXECUTOR_DEFAULT_SCHEDULER_EXECUTOR_ALLOCATOR
Stream Benchmark - Add(=)(=)=
Stream Benchmark - Scale(=)(=)(=)
Stream Benchmark - Triad(=)(=)(=)
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
HPX Commit6b6e1e795f5b31
HPX Datetime2023-03-06T15:59:25+00:002023-04-12T13:40:36+00:00
Envfile
Hostnamemedusa08.rostam.cct.lsu.edumedusa08.rostam.cct.lsu.edu
Clusternamerostamrostam
Datetime2023-03-10T03:28:29.145749-06:002023-04-12T08:51:13.218686-05:00
Compiler/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1/opt/apps/llvm/13.0.1/bin/clang++ 13.0.1

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (≤5%)
++/--Large performance improvement/degradation (≤10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks a lot! Could you please fix the clang-format issues? Otherwise LGTM.

@Pansysk75 Pansysk75 marked this pull request as draft April 12, 2023 14:06
@Pansysk75 Pansysk75 force-pushed the fix_segmented_algorithm_tests branch from 36279ac to 879ad55 Compare April 12, 2023 14:12
@Pansysk75 Pansysk75 marked this pull request as ready for review April 12, 2023 14:25
@Pansysk75 Pansysk75 requested a review from hkaiser April 12, 2023 14:55
hkaiser
hkaiser previously approved these changes Apr 12, 2023
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hkaiser
Copy link
Member

hkaiser commented Apr 12, 2023

@Pansysk75 We now could revert the change here e427e0e. This was introduced to ignore errors when running the segmented algorithm tests.

@Pansysk75
Copy link
Member Author

Pansysk75 commented Apr 12, 2023

@Pansysk75 We now could revert the change here e427e0e. This was introduced to ignore errors when running the segmented algorithm tests.

I saw some "Timeout" failures on some segmented algorithms on some past CircleCI runs. This fix does not address those failures. For example: https://app.circleci.com/pipelines/github/STEllAR-GROUP/hpx/14417/workflows/69d7cda3-1ae3-4492-b881-a39d963dd6b5/jobs/340881/tests

I would like to try fixing that as well, but I'm not sure how to approach that issue, or what its cause might be. The CircleCI error message is not very informative.
Well... there is also a 2Gb debug-log.txt, which is saved as an artifact, but I haven't gotten around to trying to parse that yet.

Let me know if you have any input on that.

@Pansysk75
Copy link
Member Author

Then there is also tests.unit.modules.runtime_components.distributed.tcp.migrate_polymorphic_component failing.
I'll use this PR as an opportunity to look into that as well.

@hkaiser
Copy link
Member

hkaiser commented Apr 12, 2023

Then there is also tests.unit.modules.runtime_components.distributed.tcp.migrate_polymorphic_component failing. I'll use this PR as an opportunity to look into that as well.

I appreciate that. Let's do this on a separate PR, however.

@hkaiser
Copy link
Member

hkaiser commented Apr 12, 2023

Well... there is also a 2Gb debug-log.txt, which is saved as an artifact, but I haven't gotten around to trying to parse that yet.

Short of reproducing this locally, the log file is all we have.

@Pansysk75 Pansysk75 force-pushed the fix_segmented_algorithm_tests branch from ca7415e to 9b7f2f5 Compare April 14, 2023 18:53
@hkaiser
Copy link
Member

hkaiser commented Apr 15, 2023

@Pansysk75 While this fixes one issue, it seems that there are more problems. Should we go ahead with merging this PR anyways?

@Pansysk75
Copy link
Member Author

Pansysk75 commented Apr 15, 2023

@Pansysk75 While this fixes one issue, it seems that there are more problems. Should we go ahead with merging this PR anyways?

@hkaiser I agree, let's merge this PR, since I don't have any insight on the "Timeout" issue for the time beign.

@hkaiser
Copy link
Member

hkaiser commented Apr 15, 2023

bors merge

@bors
Copy link

bors bot commented Apr 15, 2023

Build succeeded:

  • Bors

@bors bors bot merged commit 2c83e2d into STEllAR-GROUP:master Apr 15, 2023
51 of 61 checks passed
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.

None yet

4 participants