YARN-11844: Support configuration of retry policy on GPU discovery #7857
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
hotcodemacha
approved these changes
Aug 5, 2025
Closes apache#7857 Co-authored-by: Jayadeep Jayaraman <[email protected]> Reviewed-by: Ashutosh Gupta <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
reviewed
Aug 7, 2025
| final String msg = "Failed to execute GPU device detection script"; | ||
|
|
||
| // The default max errors is 10. Verify that it keeps going for an 11th try. | ||
| for (int i = 0; i < 11; ++i) { |
There was a problem hiding this comment.
I changed this 11 to 15 & still the test doesn't fail for me, can you check once?
There was a problem hiding this comment.
This test is covering the case where you disable the max errors by setting a negative value. To make this clearer, I dialed it up to 20 attempts, and I also added another test that sets the configuration to 11 and confirms it tries exactly 11 times.
| yarn.nodemanager.resource-plugins.gpu.discovery-max-errors. | ||
| </description> | ||
| <name>yarn.nodemanager.resource-plugins.gpu.discovery-timeout</name> | ||
| <value>10000ms</value> |
There was a problem hiding this comment.
Sounds good, updated.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@ayushtkn , thanks for the review! All comments addressed, and +1 from Yetus. How does this look now? |
cnauroth
added a commit
that referenced
this pull request
Aug 11, 2025
Closes #7857 Co-authored-by: Jayadeep Jayaraman <[email protected]> Reviewed-by: Ashutosh Gupta <[email protected]> Signed-off-by: Ayush Saxena <[email protected]> (cherry picked from commit 0f34922)
cnauroth
added a commit
that referenced
this pull request
Aug 11, 2025
Closes #7857 Co-authored-by: Jayadeep Jayaraman <[email protected]> Reviewed-by: Ashutosh Gupta <[email protected]> Signed-off-by: Ayush Saxena <[email protected]> (cherry picked from commit 0f34922) (cherry picked from commit f2b69ba)
|
I committed to trunk and merged to branch-3.4 and branch-3.3 with some minor conflicts resolved. @ayushtkn and @hotcodemacha , thank you for the reviews. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
The NodeManager invokes an external binary (e.g.
nvidia-smi) to discover attached GPUs. Right now, there is a hard-coded 10-second timeout on execution of this binary and a hard-coded max error count of 10, beyond which the NodeManager will stop attempting discovery. This change will provide new configuration properties to control both the timeout and the max errors, which is useful in environments where there may be a delay in binding the GPU to the host. Default values for the new configuration properties will be set so as to maintain the existing behavior.Special thanks to @jjayadeep06 for co-authoring this patch.
How was this patch tested?
New unit test and manual testing.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?