Fix the multi-node mpi warning - #1714
Conversation
Ramble Performance Test MetricsResults produced with commit: 2d8feb8
|
There was a problem hiding this comment.
Code Review
This pull request removes a redundant line in base_class.py that incorrectly set n_nodes to 1, and adds a corresponding unit test to verify multi-node MPI command warnings. The reviewer pointed out a potential ValueError when converting n_nodes to an integer if it is not a valid integer string, suggesting a robust try-except block to handle this case.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in parsing the n_nodes variable where it was previously always evaluated to 1 due to a buggy ternary operator. It also adds a corresponding unit test to verify warnings when running multi-node experiments without an MPI command. The reviewer suggests making the n_nodes parsing more robust by handling float strings, unexpanded placeholders, and potential parsing exceptions gracefully.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1714 +/- ##
========================================
Coverage 93.48% 93.48%
========================================
Files 367 367
Lines 35916 35924 +8
========================================
+ Hits 33575 33584 +9
+ Misses 2341 2340 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dd460f4
into
GoogleCloudPlatform:develop
No description provided.