-
Notifications
You must be signed in to change notification settings - Fork 6.7k
[Tests] Improve error message on some test-failures because of IndexError
in test_utils.py
#54343
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Daraan <[email protected]>
Signed-off-by: Daraan <[email protected]>
Signed-off-by: Daraan <[email protected]>
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.
Pull Request Overview
This PR improves error reporting in run_rllib_example_script_experiment
by guarding against an IndexError
when extracting nested exception messages and falling back to repr(e)
.
- Adds a safe extraction of
e.args[0].args[2]
with a fallback torepr(e)
if the tuple is too short. - Fixes a spelling typo in a comment (
criteris
→criteria
). - Updates the raised
RuntimeError
to include the newerrors
list.
Comments suppressed due to low confidence (1)
rllib/utils/test_utils.py:1352
- Add a unit test for this fallback branch to ensure that when nested exception
args
are too short, the code uses the fallback behavior as intended.
# Might cause a Index error if Tuple is not long enough in that case use str(e)
rllib/utils/test_utils.py
Outdated
if results.errors: | ||
# Might cause a Index error if Tuple is not long enough in that case use str(e) | ||
errors = [ | ||
e.args[0].args[2] if e.args and len(e.args[0]) > 2 else repr(e) |
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.
[nitpick] Consider using str(e)
instead of repr(e)
for a clearer, human-readable error message.
e.args[0].args[2] if e.args and len(e.args[0]) > 2 else repr(e) | |
e.args[0].args[2] if e.args and len(e.args[0]) > 2 else str(e) |
Copilot uses AI. Check for mistakes.
IndexError
in test_utils.py
Co-authored-by: Copilot <[email protected]> Signed-off-by: Daraan <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Daraan <[email protected]>
Signed-off-by: Daraan <[email protected]>
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
(ง •_•)ง stale |
Why are these changes needed?
In the CI I saw a fleaky test failing, but its error is not reported correctly:
In the logs the actual error does not appear because of the
IndexError
forf"{[e.args[0].args[2] for e in results.errors]}"
. I am not sure whye.args[0].args[2]
is used, a nested Exception perhaps? This PR adds a guard to fallback to a simplerrepr(e)
in case theIndexError
would accur again. This allows to investigate why thisIndexError
occures and raises theRuntimeError
like expected.Related issue number
NA
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.