[release] Allow release test to specify num_retries #57578
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| num_retries = test.get("run", {}).get("num_retries") | ||
| if num_retries: | ||
| step["retry"]["automatic"][0]["limit"] = num_retries |
There was a problem hiding this comment.
Bug: Retry Logic Fails When Zero Retries Specified
The if num_retries: condition evaluates to False when num_retries is 0, preventing tests from explicitly disabling retries. This results in default retry behavior instead of the intended zero retries.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature to configure the number of retries for release tests. The implementation is straightforward, but I've found one important issue in the logic that handles the num_retries value. Please see my comment below.
| cmd += ["--smoke-test"] | ||
|
|
||
| num_retries = test.get("run", {}).get("num_retries") | ||
| if num_retries: |
There was a problem hiding this comment.
The condition if num_retries: will evaluate to False when num_retries is 0. This prevents disabling retries for a test by setting num_retries: 0, which is a key use case mentioned in the PR description ("to not retry"). When num_retries is 0, the default retry limit (likely 1) will be used instead of disabling retries.
To correctly handle the case where num_retries is 0, you should check if the value is not None.
| if num_retries: | |
| if num_retries is not None: |
| if smoke_test: | ||
| cmd += ["--smoke-test"] | ||
|
|
||
| num_retries = test.get("run", {}).get("num_retries") |
ray-gardener
bot
added
core
2 participants
So it can be configured for expensive tests to not retry