-
Notifications
You must be signed in to change notification settings - Fork 13
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 miniapps as tests #1112
base: master
Are you sure you want to change the base?
Add miniapps as tests #1112
Conversation
cscs-ci run |
b395f89
to
9900e70
Compare
cscs-ci run |
1 similar comment
cscs-ci run |
7619183
to
7a85649
Compare
cscs-ci run |
1 similar comment
cscs-ci run |
7041343
to
ed29663
Compare
cscs-ci run |
1 similar comment
cscs-ci run |
6c39878
to
62b54e9
Compare
Use category label to split up CI jobs (e.g. by miniapp or unit test).
62b54e9
to
f7d52d6
Compare
cscs-ci run |
1 similar comment
cscs-ci run |
a286d1c
to
8533a59
Compare
8533a59
to
71eac9d
Compare
cscs-ci run |
cscs-ci run |
cscs-ci run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not happy about spreading pika::wait
s everywhere in the miniapps.
I'd prefer another solution.
Would scheduling and sync_wait
ing an Ibarrier work?
I don't know, but I can try. It would need |
cscs-ci run |
1 similar comment
cscs-ci run |
This reverts commit 0755556.
6bd0c5a
to
79d9861
Compare
cscs-ci run |
cscs-ci run |
1 similar comment
cscs-ci run |
5b6b907
to
2752d88
Compare
cscs-ci run |
cscs-ci run |
d2b6fd6
to
16ad0a4
Compare
cscs-ci run |
1 similar comment
cscs-ci run |
bda07a0
to
a354d3b
Compare
cscs-ci run |
for (std::size_t i = 0; i < comm_grid.num_pipelines(); ++i) { | ||
sync_wait(comm_grid.full_communicator_pipeline().exclusive()); | ||
sync_wait(comm_grid.row_communicator_pipeline().exclusive()); | ||
sync_wait(comm_grid.col_communicator_pipeline().exclusive()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for communicators does not strictly seem to be required in all the miniapps, but I would add it to all of them in any case. Whether or not it's required probably depends on the internal communication patterns.
In any case, the above snippet seems to work fine in CI now. I think we could either:
- add a helper function available only for tests/miniapps with the signature
void wait_all_communicators(CommunicatorGrid&)
which does the above, or - add a member function, similar to
Matrix::waitLocalTiles
, toCommunicatorGrid
which does the above
Do you have preferences? Other ideas? I think the latter is nice in terms of encapsulation, but I'm not sure we want to encourage users to use it (yet...?).
Adds the algorithms miniapps as tests. Does not add
miniapp_laset
as it's not really testing anything too useful, and it messes with thecheck-threads
setup (which checks if OS threads are being spawned that shouldn't be spawned). The miniapps are run with 6 ranks,--grid-rows 3 --grid-cols 2
, and otherwise default options.This splits
DLAF_addTest
into two separate functions: one which adds a test given a target (DLAF_addTargetTest
) which is used to add the miniapps since they already have CMake targets, andDLAF_addTest
which creates an executable and then callsDLAF_addTargetTest
. The behaviour ofDLAF_addTest
remains unchanged.This also adds a
CATEGORY
parameter that can be added to tests as a label. This is added alongside theRANK_*
labels. TheCATEGORY
defaults toUNIT
for "unit tests", and I set it toMINIAPP
for the miniapps. I'm fine with making the choice explicit and/or renaming/adding the categories. I then use theRANK
andCATEGORY
labels to generate jobs the same wayRANK
is used currently onmaster
. Combinations that have no tests are not added as CI jobs.Finally, many miniapps were hanging on the CUDA configurations, where they run with only one worker thread and one MPI thread. I've changed the
waitLocalTiles
calls topika::wait
calls beforeMPI_Barrier
is called to make sure that really nothing is running anymore and deadlocks are avoided. I've only changed them topika::wait
after the algorithms are called, but for consistency it might make sense to usepika::wait
anywhere beforeMPI_Barrier
is called?