Skip to content

Move ray actor pool out of experimental - #1754

Merged
oyilmaz-nvidia merged 2 commits into
NVIDIA-NeMo:mainfrom
oyilmaz-nvidia:onur/move-ray-pool-out-exp
Apr 7, 2026
Merged

Move ray actor pool out of experimental#1754
oyilmaz-nvidia merged 2 commits into
NVIDIA-NeMo:mainfrom
oyilmaz-nvidia:onur/move-ray-pool-out-exp

Conversation

@oyilmaz-nvidia

Copy link
Copy Markdown
Contributor

Summary

Promotes RayActorPoolExecutor and shared Ray backend utilities out of the experimental namespace, following the same pattern used when RayDataExecutor was promoted in #1619.

What changed

New: nemo_curator/backends/ray_actor_pool/
Moved from nemo_curator/backends/experimental/ray_actor_pool/. Contains executor.py, adapter.py, raft_adapter.py, shuffle_adapter.py, and utils.py — no logic changes.

Updated: nemo_curator/backends/utils.py

Merged the contents of nemo_curator/backends/experimental/utils.py into the existing backends/utils.py. This includes RayStageSpecKeys, get_head_node_id, get_available_cpu_gpu_resources, execute_setup_on_node, and related helpers. The experimental/utils.py file is deleted.

Deleted: nemo_curator/backends/experimental/

The entire experimental/ directory is removed. Both ray_data and ray_actor_pool are now fully promoted.

Import updates across the codebase

nemo_curator/backends/ray_data/ — 3 files updated to import from backends.utils instead of backends.experimental.utils
nemo_curator/stages/ — 17 files updated (RayStageSpecKeys and RayActorPoolExecutor imports)
tests/ — 12 files updated; tests/backends/experimental/ deleted, tests moved to tests/backends/ray_actor_pool/ and merged into tests/backends/test_utils.py
tutorials/synthetic/nemo_data_designer/ — .py and .ipynb updated
benchmarking/scripts/utils.py — updated

Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Apr 7, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
@oyilmaz-nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test ce3f7ca

@oyilmaz-nvidia
oyilmaz-nvidia enabled auto-merge (squash) April 7, 2026 17:18
@greptile-apps

greptile-apps Bot commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR promotes RayActorPoolExecutor and shared Ray backend utilities out of the experimental namespace, following the same pattern as #1619. The changes are purely structural — import paths updated across ~50 files, experimental/utils.py merged into backends/utils.py, and the experimental/ directory deleted — with no logic changes.

  • P1: raft_adapter.py setup() has a bare multi-line string where line 144 is discarded, truncating the RuntimeError message (fix: wrap in parentheses).
  • P2: _cleanup_actor_pool iterates _future_to_actor in the wrong direction, extracting ObjectRefs instead of ActorHandles — harmless in practice because the dict is always empty at cleanup time, but semantically incorrect.

Confidence Score: 4/5

Safe to merge after fixing the truncated RuntimeError message in raft_adapter.py setup()

One P1 defect: setup() in raft_adapter.py raises a RuntimeError with a truncated message due to a missing parenthesis wrap around a multi-line string literal. One P2 concern: _cleanup_actor_pool iterates _future_to_actor in the wrong direction but is never triggered in normal execution. The rest of the PR is a clean, well-structured namespace promotion.

nemo_curator/backends/ray_actor_pool/raft_adapter.py (P1 string bug); nemo_curator/backends/ray_actor_pool/executor.py (P2 wrong iteration direction)

Important Files Changed

Filename Overview
nemo_curator/backends/ray_actor_pool/raft_adapter.py Moved from experimental; setup() has a string continuation bug that truncates the RuntimeError message
nemo_curator/backends/ray_actor_pool/executor.py Moved from experimental; _cleanup_actor_pool incorrectly iterates _future_to_actor extracting futures instead of actor handles (harmless in practice due to empty dict at cleanup time)
nemo_curator/backends/utils.py Cleanly merges RayStageSpecKeys, get_head_node_id, execute_setup_on_node, and helpers from experimental/utils.py into the main backends utility module
nemo_curator/backends/ray_actor_pool/init.py New init module for promoted package, cleanly exporting RayActorPoolExecutor
tests/backends/test_utils.py Migrated tests from experimental/test_utils.py; properly resets _HEAD_NODE_ID_CACHE between tests via fixture
tests/backends/ray_actor_pool/test_executor.py New test file covering _parse_runtime_env; correctly tests both override and default cases

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph OLD["Before — experimental/"]
        A["backends/experimental/ray_actor_pool/\nexecutor.py · raft_adapter.py\nadapter.py · shuffle_adapter.py · utils.py"]
        B["backends/experimental/utils.py\nRayStageSpecKeys\nget_head_node_id\nexecute_setup_on_node"]
        C["tests/backends/experimental/\nray_actor_pool/test_executor.py\ntest_utils.py"]
    end
    subgraph NEW["After — stable"]
        D["backends/ray_actor_pool/\nexecutor.py · raft_adapter.py\nadapter.py · shuffle_adapter.py · utils.py"]
        E["backends/utils.py\n← merged RayStageSpecKeys\nget_head_node_id\nexecute_setup_on_node"]
        F["tests/backends/ray_actor_pool/\ntest_executor.py"]
        G["tests/backends/test_utils.py\n← merged tests"]
    end
    A -->|promoted| D
    B -->|merged into| E
    C -->|moved| F
    C -->|merged into| G
    H["~35 import sites\nstages · tests · tutorials · benchmarks"] -->|import paths updated| D & E
Loading

Comments Outside Diff (2)

  1. nemo_curator/backends/ray_actor_pool/raft_adapter.py, line 143-144 (link)

    P1 Truncated error message — second string literal is silently discarded

    Line 144 is a standalone statement, not a continuation of msg. Python only performs implicit string concatenation when string literals are adjacent inside parentheses/brackets, not across bare assignments. As written, msg contains only "The unique ID of root is not set. Make sure \broadcast_root_unique_id` "and the second half is lost at the point theRuntimeError` is raised.

  2. nemo_curator/backends/ray_actor_pool/executor.py, line 381 (link)

    P2 _future_to_actor iteration extracts futures, not actor handles

    ActorPool._future_to_actor maps ObjectRef → ActorHandle, so for actor, _ in _future_to_actor.items() binds actor to the key (an ObjectRef) and _ to the value (the actual ActorHandle). In practice _future_to_actor is empty at cleanup time because map_unordered drains all futures before _cleanup_actor_pool is called, so no error occurs today — but if the dict were ever non-empty it would silently pass futures to ray.kill() instead of actor handles.

Reviews (1): Last reviewed commit: "Fix linting issues" | Re-trigger Greptile

@oyilmaz-nvidia

Copy link
Copy Markdown
Contributor Author

/claude review

@oyilmaz-nvidia
oyilmaz-nvidia disabled auto-merge April 7, 2026 17:33
@oyilmaz-nvidia
oyilmaz-nvidia enabled auto-merge (squash) April 7, 2026 17:34
@oyilmaz-nvidia
oyilmaz-nvidia merged commit 7fc9de4 into NVIDIA-NeMo:main Apr 7, 2026
50 checks passed
ayushdg pushed a commit that referenced this pull request Apr 21, 2026
* Move ray actor pool out of experimental

Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>

* Fix linting issues

Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>

---------

Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
Signed-off-by: Ayush Dattagupta <ayushdg95@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants