[core] Split task option helpers from _raylet.pyx#63124
Conversation
Signed-off-by: hieuvous <trunghieu101112a1@gmail.com>
Signed-off-by: chichic21039 <Linhpham.1508055@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request refactors task option utility functions by moving them from python/ray/_raylet.pyx to a new include file python/ray/includes/task_options_utils.pxi. The review identified several issues in the moved code: a vague error message in prepare_actor_concurrency_groups, a missing except -1 clause for proper exception propagation in Cython, and efficiency and correctness problems in prepare_resources related to redundant list construction and malformed ValueError arguments.
| c_vector[CFunctionDescriptor] c_fd_list | ||
|
|
||
| if concurrency_groups_dict is None: | ||
| raise ValueError("Must provide it...") |
| cdef int prepare_actor_concurrency_groups( | ||
| dict concurrency_groups_dict, | ||
| c_vector[CConcurrencyGroup] *concurrency_groups): |
There was a problem hiding this comment.
The function prepare_actor_concurrency_groups is missing an except clause. In Cython, cdef functions that return a primitive type and can raise Python exceptions should be marked with except -1 (or similar) to ensure proper exception propagation to the caller. Without it, exceptions might be swallowed or only print a warning to stderr.
cdef int prepare_actor_concurrency_groups(
dict concurrency_groups_dict,
c_vector[CConcurrencyGroup] *concurrency_groups) except -1:
| cdef int prepare_resources( | ||
| dict resource_dict, | ||
| unordered_map[c_string, double] *resource_map) except -1: | ||
| cdef: | ||
| list unit_resources | ||
|
|
||
| if resource_dict is None: | ||
| raise ValueError("Must provide resource map.") | ||
|
|
||
| resource_map[0].reserve(len(resource_dict)) | ||
| for key, value in resource_dict.items(): | ||
| if not (isinstance(value, int) or isinstance(value, float)): | ||
| raise ValueError("Resource quantities may only be ints or floats.") | ||
| if value < 0: | ||
| raise ValueError("Resource quantities may not be negative.") | ||
| if value > 0: | ||
| unit_resources = ( | ||
| f"{RayConfig.instance().predefined_unit_instance_resources()\ | ||
| .decode('utf-8')}," | ||
| f"{RayConfig.instance().custom_unit_instance_resources()\ | ||
| .decode('utf-8')}" | ||
| ).split(",") | ||
|
|
||
| if (value >= 1 and isinstance(value, float) | ||
| and not value.is_integer() and str(key) in unit_resources): | ||
| raise ValueError( | ||
| f"{key} resource quantities >1 must", | ||
| f" be whole numbers. The specified quantity {value} is invalid.") | ||
| resource_map[0][key.encode("ascii")] = float(value) | ||
| return 0 No newline at end of file |
There was a problem hiding this comment.
There are two issues in prepare_resources that should be addressed:
- Efficiency: The
unit_resourceslist is reconstructed in every iteration of the loop (lines 134-139). Since these configuration values are constant for the duration of the call, the list should be moved outside the loop to avoid redundant calculations and string operations. - Correctness: The
ValueErrorat lines 143-145 is called with two separate string arguments due to the trailing comma on line 144. This results in the exception message being a tuple of strings (e.g.,('msg1', 'msg2')) instead of a single concatenated string.
cdef int prepare_resources(
dict resource_dict,
unordered_map[c_string, double] *resource_map) except -1:
cdef:
list unit_resources
if resource_dict is None:
raise ValueError("Must provide resource map.")
unit_resources = (
f"{RayConfig.instance().predefined_unit_instance_resources().decode('utf-8')},"
f"{RayConfig.instance().custom_unit_instance_resources().decode('utf-8')}"
).split(",")
resource_map[0].reserve(len(resource_dict))
for key, value in resource_dict.items():
if not (isinstance(value, int) or isinstance(value, float)):
raise ValueError("Resource quantities may only be ints or floats.")
if value < 0:
raise ValueError("Resource quantities may not be negative.")
if value > 0:
if (value >= 1 and isinstance(value, float)
and not value.is_integer() and str(key) in unit_resources):
raise ValueError(
f"{key} resource quantities >1 must "
f"be whole numbers. The specified quantity {value} is invalid.")
resource_map[0][key.encode("ascii")] = float(value)
return 0
Signed-off-by: chichic21039 <Linhpham.1508055@gmail.com>
edoakes
left a comment
There was a problem hiding this comment.
I've triggered the premerge CI build: https://buildkite.com/ray-project/premerge/builds/65856
| cdef int prepare_labels( | ||
| dict label_dict, | ||
| unordered_map[c_string, c_string] *label_map) except -1: |
There was a problem hiding this comment.
please add docstring comments to the functions as you move them
There was a problem hiding this comment.
Thanks for the review, I added docstring-style comments for the moved task option helper functions
There was a problem hiding this comment.
Small follow-up: I also addressed the bot feedback by restoring the original ValueError arguments, so the latest commit should preserve the original behavior. Most checks look good now, but the Buildkite premerge still seems to need maintainer input/approval. Could you please trigger/rerun it for the latest commit when you get a chance? 🙏
Signed-off-by: chichic21039 <Linhpham.1508055@gmail.com>
3396a8f to
ec8331d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit ec8331d. Configure here.
Signed-off-by: chichic21039 <Linhpham.1508055@gmail.com>
|
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. |

Why are these changes needed?
This PR addresses #63082, which is a scoped sub-issue related to #51080 after the discussion with @edoakes.
python/ray/_raylet.pyxis a large Cython file that contains multiple groups of helper functions. Some helper functions related to task option preparation, including labels, resource preparation, fallback strategy, function descriptors, and actor concurrency groups, were previously defined directly inside_raylet.pyx.This PR moves those task option helper functions into a focused Cython include file:
python/ray/includes/task_options_utils.pxiThis is a behavior-preserving refactor. It does not aim to change runtime behavior or improve performance directly. The main goal is to improve maintainability by reducing the size of
_raylet.pyx, grouping related task option helper logic in one place, and making future changes easier to review and extend.What changed?
This PR:
python/ray/includes/task_options_utils.pxipython/ray/_raylet.pyx_raylet.pyxto include the new.pxifileThe moved helper functions include task option preparation logic such as label handling, resource preparation, fallback strategy handling, function descriptor preparation, and actor concurrency group preparation.
Design rationale
The change keeps the refactor small and focused. Instead of changing task submission behavior or modifying call sites, this PR only relocates a coherent group of helper functions into a dedicated include file.
Although this only extracts a small portion of
_raylet.pyx, the extracted code forms a related helper group for task option preparation. Keeping the PR scoped makes it easier to review, lowers the risk of behavioral regressions, and provides a clear pattern for future extractions from the large Cython file.Related issue number
Closes #63082
Related to #51080
Checks
git diff --checkpython/ray/_raylet.pyxandpython/ray/includes/task_options_utils.pxi_raylet.pyxincludestask_options_utils.pxiexactly oncepip install -e . --verbosefrompython/Docs and tests
No user-facing documentation was added because this is an internal behavior-preserving refactor.
No new tests were added because the PR does not introduce new functionality or change existing behavior. The local editable build/install completed successfully with
pip install -e . --verbose.Team contribution note
This PR was prepared as a team contribution for the scoped task option helper refactor.
The work was discussed and split by helper-function groups among team members. The final clean PR branch contains the combined implementation in two signed-off commits:
ec32208: split task option helper logic intotask_options_utils.pxi5d44e1f: move resource and fallback task option helpers intotask_options_utils.pxiSome intermediate work was done on separate team branches before being consolidated into this final PR branch to keep the upstream PR small, clean, and reviewable.
Notes for reviewers
This PR was prepared after the scoped discussion in #51080 and tracked through #63082.
The work was merged into the final PR branch:
split-task-options-utils-pr