[core] Split raylet cython file into multiple files (DynamicObjectRefGenerator) #52095

machichima · 2025-04-08T14:42:58Z

Why are these changes needed?

As mentioned in #51080, separate DynamicObjectRefGenerator class from the large _raylet.pyx file.

Related issue number

Closes #52029

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

-e Signed-off-by: machichima <[email protected]>

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

machichima · 2025-04-08T14:48:06Z

Hi @dentiny ,

I'm new to cython and might make some mistake here. Please have a look and let me know if anything should be modified.

Thanks!

dentiny · 2025-04-08T17:08:53Z

I think you need to change all python caller side?

machichima · 2025-04-09T14:21:12Z

It seems like the import error happens in _raylet.pyx. As originally the class is defined as the python style, so I assume that it is fine to keep it like so, only create .pyx file and use import in raylet.pyx?

[2025-04-08T15:10:13Z] Traceback (most recent call last):
[2025-04-08T15:10:13Z]   File "/root/.cache/bazel/_bazel_root/1df605deb6d24fc8068f6e25793ec703/execroot/com_github_ray_project_ray/bazel-out/k8-opt/bin/python/ray/autoscaler/v2/tests/test_sdk.runfiles/com_github_ray_project_ray/python/ray/autoscaler/v2/tests/test_sdk.py", line 11, in <module>
[2025-04-08T15:10:13Z]     import ray
[2025-04-08T15:10:13Z]   File "/rayci/python/ray/__init__.py", line 85, in <module>
[2025-04-08T15:10:13Z]     import ray._raylet  # noqa: E402
[2025-04-08T15:10:13Z]   File "python/ray/_raylet.pyx", line 1, in init ray._raylet
[2025-04-08T15:10:13Z] ModuleNotFoundError: No module named 'ray.cython_components.object_ref_generator'

-e Signed-off-by: machichima <[email protected]>

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

-e Signed-off-by: machichima <[email protected]>

machichima · 2025-04-12T13:57:11Z

Hi @dentiny ,
Could you please have a look at this? I put it in a .py file as the class is simply a python class, not cython

Thanks!

dentiny

There're two main purpose for the issue (split classes and functions out of the giant cython file as possible):

(major) speed up compilation speed, it's a painpoint for any dev work related to FFI
(minor) increase code maintainability by moving some pieces out of the file (you could tell the file is so large)

I think your change fits in the second purpose, so it looks ok to me.
CC @jjyao / @edoakes / @dayshah if you have other thoughts.

edoakes · 2025-04-14T15:15:00Z

python/ray/cython_components/object_ref_generator.py

@@ -0,0 +1,17 @@
+from ray.util.annotations import DeveloperAPI


this is not cython code so there's no need to put it under cython_components. how about just ray/_private/dynamic_object_ref_generator.py?

Sure! Just moved it to _private.
Thanks!

-e Signed-off-by: machichima <[email protected]>

edoakes · 2025-04-14T16:35:44Z

python/ray/_private/object_ref_generator.py

+    def __init__(self, refs):
+        # TODO(swang): As an optimization, can also store the generator
+        # ObjectID so that we don't need to keep individual ref counts for the
+        # inner ObjectRefs.
+        self._refs = refs
+
+    def __iter__(self):
+        for ref in self._refs:
+            yield ref
+
+    def __len__(self):
+        return len(self._refs)


while you're at it, do you mind adding type hints for this code?

Hi @edoakes ,
I just tried and notice that we cannot import ObjectRef here, as this is the .py file, and from ray import ObjectRef cause the circular import. Would you mind if I use list[Any] here for refs? Or is there any way that I can use ObjectRef as type here?

Thanks!

I think you can use a string type hint instead: List["ray.ObjectRef"]

@edoakes I just added the type hint. PTAL
Thanks!

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

-e Signed-off-by: machichima <[email protected]>

edoakes · 2025-04-18T18:10:14Z

thanks @machichima !

…Generator) (ray-project#52095) As mentioned in ray-project#51080, separate `DynamicObjectRefGenerator` class from the large `_raylet.pyx` file. Closes ray-project#52029

wingkitlee0 · 2025-05-03T21:43:16Z

Sorry for leaving comment in a merged PR. This could be just a minor change..
Just curious if it's intentional that the while-loop is replaced by a for-loop:

while self._refs:
  yield self._refs.pop(0)

I am asking this because the len/size changes after each iteration/next() in the old behavior, while it keeps the full list and keeps the len in the new behavior.
Happy to open a new issue for discussion.

dayshah · 2025-05-03T22:47:55Z

Sorry for leaving comment in a merged PR. This could be just a minor change.. Just curious if it's intentional that the while-loop is replaced by a for-loop:
while self._refs:
  yield self._refs.pop(0)
I am asking this because the len/size changes after each iteration/next() in the old behavior, while it keeps the full list and keeps the len in the new behavior. Happy to open a new issue for discussion.

cc @edoakes not that familiar with how DynamicObjectRefGenerator works, but any concern of the object sticking around longer in obj store bc the ref sticks around in the list without the pop?

…Generator) (ray-project#52095) As mentioned in ray-project#51080, separate `DynamicObjectRefGenerator` class from the large `_raylet.pyx` file. Closes ray-project#52029

dayshah · 2025-05-07T19:41:42Z

Sorry for leaving comment in a merged PR. This could be just a minor change.. Just curious if it's intentional that the while-loop is replaced by a for-loop:
while self._refs:
  yield self._refs.pop(0)
I am asking this because the len/size changes after each iteration/next() in the old behavior, while it keeps the full list and keeps the len in the new behavior. Happy to open a new issue for discussion.

@wingkitlee0 I confirmed this did introduce an actual bug and opened a pr to fix it. Thanks a lot for noticing it and bringing it up and being a great member of the Ray community!

In #52095, the while loop in DynamicObjectRefGenerator that pops the ref was just changed to a for loop that iterates through the refs. This results in the refs sticking around and using up memory when they can be destroyed. Also changing the list to a deque for efficiency on popping from the front. --------- Signed-off-by: dayshah <[email protected]>

…Generator) (ray-project#52095) As mentioned in ray-project#51080, separate `DynamicObjectRefGenerator` class from the large `_raylet.pyx` file. Closes ray-project#52029 Signed-off-by: zhaoch23 <[email protected]>

In ray-project#52095, the while loop in DynamicObjectRefGenerator that pops the ref was just changed to a for loop that iterates through the refs. This results in the refs sticking around and using up memory when they can be destroyed. Also changing the list to a deque for efficiency on popping from the front. --------- Signed-off-by: dayshah <[email protected]> Signed-off-by: zhaoch23 <[email protected]>

In ray-project#52095, the while loop in DynamicObjectRefGenerator that pops the ref was just changed to a for loop that iterates through the refs. This results in the refs sticking around and using up memory when they can be destroyed. Also changing the list to a deque for efficiency on popping from the front. --------- Signed-off-by: dayshah <[email protected]> Signed-off-by: Vicky Tsang <[email protected]>

machichima added 2 commits April 8, 2025 22:38

refactor: move DynamicObjectRefGenerator to separate file

92f3a97

-e Signed-off-by: machichima <[email protected]>

Merge branch 'master' of github.com:ray-project/ray into 52029-split-…

8bf561a

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

hainesmichaelc added the community-contribution Contributed by the community label Apr 9, 2025

fix: update cimport to import

42183d3

-e Signed-off-by: machichima <[email protected]>

jcotant1 added the core Issues that should be addressed in Ray Core label Apr 10, 2025

machichima and others added 6 commits April 12, 2025 13:18

Merge branch 'master' of github.com:ray-project/ray into 52029-split-…

c9260f0

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

fix: DynamicObjectRefGenerator to py file

4b6e92d

-e Signed-off-by: machichima <[email protected]>

Merge branch 'master' into 52029-split-cython-dynamic-obj-ref-gen

2179700

fix: add api annotation

02ec294

-e Signed-off-by: machichima <[email protected]>

style: lint

7ecc232

-e Signed-off-by: machichima <[email protected]>

fix: make cython_components into module

cd97ed6

-e Signed-off-by: machichima <[email protected]>

dentiny added the go add ONLY when ready to merge, run all tests label Apr 12, 2025

dentiny reviewed Apr 12, 2025

View reviewed changes

machichima added 3 commits April 13, 2025 10:18

Merge branch 'master' into 52029-split-cython-dynamic-obj-ref-gen

eec23ee

Merge branch 'master' into 52029-split-cython-dynamic-obj-ref-gen

f0257a5

Merge branch 'master' into 52029-split-cython-dynamic-obj-ref-gen

9f6f924

edoakes reviewed Apr 14, 2025

View reviewed changes

refactor: move file to _private

372d513

-e Signed-off-by: machichima <[email protected]>

edoakes reviewed Apr 14, 2025

View reviewed changes

machichima added 4 commits April 15, 2025 20:41

Merge branch 'master' of github.com:ray-project/ray into 52029-split-…

05c3478

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

Merge branch 'master' of github.com:ray-project/ray into 52029-split-…

7e6998f

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

Merge branch 'master' of github.com:ray-project/ray into 52029-split-…

4a41b68

…cython-dynamic-obj-ref-gen -e Signed-off-by: machichima <[email protected]>

refactor: add type hints

d894486

-e Signed-off-by: machichima <[email protected]>

machichima force-pushed the 52029-split-cython-dynamic-obj-ref-gen branch from 98a3c5f to d894486 Compare April 17, 2025 14:01

machichima and others added 2 commits April 17, 2025 22:12

fix: type checking import ray

aa610c4

-e Signed-off-by: machichima <[email protected]>

Merge branch 'master' into 52029-split-cython-dynamic-obj-ref-gen

fc27b72

edoakes approved these changes Apr 18, 2025

View reviewed changes

edoakes merged commit ca826ec into ray-project:master Apr 18, 2025
5 checks passed

machichima deleted the 52029-split-cython-dynamic-obj-ref-gen branch April 18, 2025 23:14

dayshah mentioned this pull request May 7, 2025

[core] Correctly release dynamic generator refs #52843

Merged

8 tasks

hainesmichaelc added the community-backlog label May 22, 2025

		@@ -0,0 +1,17 @@
		from ray.util.annotations import DeveloperAPI

[core] Split raylet cython file into multiple files (DynamicObjectRefGenerator) #52095

[core] Split raylet cython file into multiple files (DynamicObjectRefGenerator) #52095

Uh oh!

Conversation

machichima commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

machichima commented Apr 8, 2025

Uh oh!

dentiny commented Apr 8, 2025

Uh oh!

machichima commented Apr 9, 2025

Uh oh!

machichima commented Apr 12, 2025

Uh oh!

dentiny left a comment

Choose a reason for hiding this comment

Uh oh!

edoakes Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

machichima Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoakes Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

machichima Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

machichima Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

edoakes Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

machichima Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edoakes commented Apr 18, 2025

Uh oh!

wingkitlee0 commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dayshah commented May 3, 2025

Uh oh!

dayshah commented May 7, 2025

Uh oh!

Uh oh!

machichima commented Apr 8, 2025 •

edited

Loading

machichima Apr 14, 2025 •

edited

Loading

wingkitlee0 commented May 3, 2025 •

edited

Loading