-
Notifications
You must be signed in to change notification settings - Fork 38
Add tag to avoid vernier CCE MPI bug #261
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 tag to avoid vernier CCE MPI bug #261
Conversation
| name = 'Vernier' | ||
| if ( LPROF ) then | ||
| call vernier_init( communicator%get_comm_mpi_val() ) | ||
| call vernier_init( communicator%get_comm_mpi_val(), tag='lfric' ) |
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.
Would it make more sense to pass the application_name through as the tag to vernier?
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.
It could do, though this was just to get the workaround on trunk quick as opposed to a perfect solution.
Ideally we wouldn't worry at all about the tags.
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.
I think changing 'lfric' to trim(application_name) would make sense for this fix, just in case we get stuck using a hard-coded name for the long term if we never get around to reverting it
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.
Given its right there in the timing_mod anyway, I don't mind adding it.
@andrewcoughtrie, does this work as an alternative to 'lfric' ?
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.
Running the tests now
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.
Happy with the way this is implemented - passing on to CR @andrewcoughtrie
andrewcoughtrie
left a comment
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.
This all looks good to me.
andrewcoughtrie
left a comment
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.
All is good and the tests pass.
Officiate the workaround so that Vernier can work out of the box at HoT without manual adjustments by users.
PR Summary
Sci/Tech Reviewer: @EdHone
Code Reviewer: @andrewcoughtrie
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_core - add_vernier_tag/run5
Suite Information
Task Information
✅ succeeded tasks - 370
Test Suite Results - lfric_apps - hot_testing/run6
Suite Information
Task Information
❌ failed tasks - 3
Outputs produced:

Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review