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.

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

bugfix: change test to +p 2 +vp 8, because +p 2 +vp 1 is nonsense #3802

Closed
wants to merge 1 commit into from
Closed

bugfix: change test to +p 2 +vp 8, because +p 2 +vp 1 is nonsense #3802

wants to merge 1 commit into from

Conversation

ericjbohm
Copy link
Contributor

Fix an old typo in the makefile for the megaampi test.

@ericjbohm ericjbohm added AMPI Issues affecting AMPI Build & test automation labels Apr 10, 2024
@ericjbohm ericjbohm added this to the 7.1 milestone Apr 10, 2024
@ericjbohm ericjbohm self-assigned this Apr 10, 2024
@matthiasdiener
Copy link
Contributor

matthiasdiener commented Apr 10, 2024 •
edited
Loading

migration/Makefile and creduce/Makefile seem to have a similar issue.

@stwhite91
Copy link
Collaborator

+p2 +vp1 is often the most simple way to reproduce a migration/PUP bug in AMPI. I don't have a problem with removing it from megampi so long as we still have it in tests/ampi/migration though. Also, +p2 +vp8 shouldn't add anything meaningful to the testing that +p2 +vp4 isn't already covering so I'd rather just remove the +p2 +vp1 and not add anything new.

@ericjbohm
Copy link
Contributor Author

+p2 +vp1 is often the most simple way to reproduce a migration/PUP bug in AMPI. I don't have a problem with removing it from megampi so long as we still have it in tests/ampi/migration though. Also, +p2 +vp8 shouldn't add anything meaningful to the testing that +p2 +vp4 isn't already covering so I'd rather just remove the +p2 +vp1 and not add anything new.

I finally got back to looking at this. There seems to be an issue with this independent from anything going on in CXI. A multicore build running on the headnode of frontier has a similar crash in the pieglobals-f90 +p1 +vp2 +balancer RandCentLB.

So, I think there is a more fundamental issue with how that interacts with newer toolchains. For reference, that bug was with PrgEnv-gnu/8.3.3 loaded. AMPI is of course totally unstable with the cray compiler, but even GNU is having issues with this test.

@stwhite91
Copy link
Collaborator

pieglobals-f90 +p1 +vp2 +balancer RandCentLB

I assume you mean +p2 +vp1

@ericjbohm
Copy link
Contributor Author

ericjbohm commented May 16, 2024 via email

@ericjbohm ericjbohm requested a review from lvkale May 16, 2024 16:28
@ericjbohm ericjbohm removed this from the 7.1 milestone Jun 5, 2024
@ericjbohm
Copy link
Contributor Author

closing this, will open an issue regarding some oddities with AMPI virtualiation

@ericjbohm ericjbohm closed this Jun 6, 2024
@stwhite91 stwhite91 deleted the fix-megaampi-make-test branch July 24, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiasdiener matthiasdiener Awaiting requested review from matthiasdiener

@stwhite91 stwhite91 Awaiting requested review from stwhite91

@evan-charmworks evan-charmworks Awaiting requested review from evan-charmworks

@lvkale lvkale Awaiting requested review from lvkale

Assignees

@ericjbohm ericjbohm

Labels
AMPI Issues affecting AMPI Build & test automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ericjbohm @matthiasdiener @stwhite91 @ebohm