Skip to content

Conversation

@daiping8
Copy link
Contributor

Description

  • Change the caught exception type from IndexError to TypeError
  • This modification ensures that the correct exception is raised when the expected accelerator ID is not included in the accelerator_visible_list

list.index will raise a ValueError if there is no such item.

https://docs.python.org/3/tutorial/datastructures.html

image

Now, the error logs will be correctly captured and printed.
image

@daiping8 daiping8 requested a review from a team as a code owner October 29, 2025 01:35
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to fix an incorrect exception type being caught for an accelerator ID visibility check. The original code was catching IndexError, but list.index() raises ValueError when an item is not found. While the intent is correct, the change to catch TypeError is also incorrect for this scenario. My review provides a correction to catch the appropriate ValueError.

@daiping8
Copy link
Contributor Author

cc @hipudding @jjyao

- Change the caught exception type from IndexError to ValueError
- This modification ensures that the correct exception is raised when the expected accelerator ID is not included in the accelerator_visible_list

Change-Id: Ie2132d3f47ca4511c1c6b53a125f37073f943f07
Signed-off-by: daiping8 <[email protected]>
@daiping8 daiping8 changed the title [Core][Channel] Fix the exception type for accelerator ID visibility check [Core][Channel] Fix the exception type for accelerator ID visibility check in accelerator_context.py Oct 29, 2025
@ray-gardener ray-gardener bot added core Issues that should be addressed in Ray Core community-contribution Contributed by the community labels Oct 29, 2025
Copy link
Collaborator

@edoakes edoakes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. Is it possible to add a test for this? I'm not sure if we have tests for the accelerator context already

@jjyao jjyao added the go add ONLY when ready to merge, run all tests label Oct 30, 2025
@edoakes edoakes merged commit a69004e into ray-project:master Oct 30, 2025
6 checks passed
YoussefEssDS pushed a commit to YoussefEssDS/ray that referenced this pull request Nov 8, 2025
…check in accelerator_context.py (ray-project#58269)

## Description

- Change the caught exception type from IndexError to TypeError
- This modification ensures that the correct exception is raised when
the expected accelerator ID is not included in the
accelerator_visible_list

`list.index` will raise a
[ValueError](https://docs.python.org/3/library/exceptions.html#ValueError)
if there is no such item.

https://docs.python.org/3/tutorial/datastructures.html

<img width="797" height="79" alt="image"
src="https://github.com/user-attachments/assets/830cf4aa-d9cb-44d3-9363-8e5cd576bae9"
/>

Now, the error logs will be correctly captured and printed.
<img width="1454" height="143" alt="image"
src="https://github.com/user-attachments/assets/2b0ed0aa-60c7-49c3-84b0-7f0d4f1ebe48"
/>

Signed-off-by: daiping8 <[email protected]>
landscapepainter pushed a commit to landscapepainter/ray that referenced this pull request Nov 17, 2025
…check in accelerator_context.py (ray-project#58269)

## Description

- Change the caught exception type from IndexError to TypeError
- This modification ensures that the correct exception is raised when
the expected accelerator ID is not included in the
accelerator_visible_list

`list.index` will raise a
[ValueError](https://docs.python.org/3/library/exceptions.html#ValueError)
if there is no such item.

https://docs.python.org/3/tutorial/datastructures.html

<img width="797" height="79" alt="image"
src="https://github.com/user-attachments/assets/830cf4aa-d9cb-44d3-9363-8e5cd576bae9"
/>

Now, the error logs will be correctly captured and printed.
<img width="1454" height="143" alt="image"
src="https://github.com/user-attachments/assets/2b0ed0aa-60c7-49c3-84b0-7f0d4f1ebe48"
/>

Signed-off-by: daiping8 <[email protected]>
Aydin-ab pushed a commit to Aydin-ab/ray-aydin that referenced this pull request Nov 19, 2025
…check in accelerator_context.py (ray-project#58269)

## Description

- Change the caught exception type from IndexError to TypeError
- This modification ensures that the correct exception is raised when
the expected accelerator ID is not included in the
accelerator_visible_list

`list.index` will raise a
[ValueError](https://docs.python.org/3/library/exceptions.html#ValueError)
if there is no such item.

https://docs.python.org/3/tutorial/datastructures.html

<img width="797" height="79" alt="image"
src="https://github.com/user-attachments/assets/830cf4aa-d9cb-44d3-9363-8e5cd576bae9"
/>

Now, the error logs will be correctly captured and printed.
<img width="1454" height="143" alt="image"
src="https://github.com/user-attachments/assets/2b0ed0aa-60c7-49c3-84b0-7f0d4f1ebe48"
/>

Signed-off-by: daiping8 <[email protected]>
Signed-off-by: Aydin Abiar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants