Skip to content

fix: skip RayVirtualCluster.shutdown when Ray already torn down - #3148

Merged
yuki-97 merged 1 commit into
mainfrom
fix/async-grpo-shutdown-crash
Jul 14, 2026
Merged

fix: skip RayVirtualCluster.shutdown when Ray already torn down#3148
yuki-97 merged 1 commit into
mainfrom
fix/async-grpo-shutdown-crash

Conversation

@ZhiyuLi-Nvidia

@ZhiyuLi-Nvidia ZhiyuLi-Nvidia commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Issue

MOPD nightly (llm_mopd_qwen3_1_7b_3n8g_megatron_pack) exits 1 after training completes cleanly:

[C] core_worker_process.cc:88: Check failed: !core_worker_process

Ray's atexit tears down CoreWorker first, then Python's GC runs __del__ on RayVirtualCluster instances, which call remove_placement_group on the dead runtime — CoreWorker re-init fails the CHECK and the process aborts.

Fix

Return early from RayVirtualCluster.shutdown() when ray.is_initialized() is False. The guard only fires from __del__ during _Py_Finalize; placement groups are already gone with Ray at that point.

5 lines, 1 file. No new imports, no semantic change when Ray is alive, no reference retention.

Verified twice end-to-end on the mopd-qwen3-1.7b-3n8g-megatron-pack nightly recipe:

🤖 Generated with Claude Code

@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia requested review from a team as code owners July 9, 2026 23:46
@copy-pr-bot

copy-pr-bot Bot commented Jul 9, 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.

@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia force-pushed the fix/async-grpo-shutdown-crash branch 2 times, most recently from 01da262 to 76a9892 Compare July 10, 2026 00:13
@ZhiyuLi-Nvidia ZhiyuLi-Nvidia added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Jul 10, 2026
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 76a9892

@yuki-97 yuki-97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks for root cause and helping fix!

I have some thoughts on whether we can scope down the fix, since other recipes run with run_grpo_nemo_gym.py doesn't meet this error. could you help to take a look?

Comment thread nemo_rl/distributed/virtual_cluster.py Outdated
@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia force-pushed the fix/async-grpo-shutdown-crash branch from 76a9892 to 5d47e13 Compare July 13, 2026 21:01
@ZhiyuLi-Nvidia ZhiyuLi-Nvidia changed the title fix: shut down Ray-owning resources before Py_Finalize fix: register RayVirtualCluster.shutdown via atexit to prevent shutdown crash Jul 13, 2026
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 5d47e13

@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia force-pushed the fix/async-grpo-shutdown-crash branch from 5d47e13 to 5dacef4 Compare July 13, 2026 21:07
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 5dacef4

@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

thanks for root cause and helping fix!

I have some thoughts on whether we can scope down the fix, since other recipes run with run_grpo_nemo_gym.py doesn't meet this error. could you help to take a look?

That's consistent with what we observed. The failure fires only when a Ray-touching del runs after Ray's atexit tears down CoreWorker, and whether that condition is reached seems to depend on cluster count and topology. Colocated GRPO has 1 RayVirtualCluster and stays silent. Non-colocated 2-cluster async GRPO stays silent for the topologies exercised so far. MOPD with teacher_cluster runs with 3 clusters and tips Ray past whatever it silently tolerates, so the CHECK fires.

MOPD nightly (llm_mopd_qwen3_1_7b_3n8g_megatron_pack) exits 1 after
training completes cleanly:

    [C] core_worker_process.cc:88: Check failed: !core_worker_process

Ray's atexit tears down CoreWorker first, then Python's GC runs __del__
on RayVirtualCluster instances, which call remove_placement_group on
the dead runtime — CoreWorker re-init fails the CHECK and the process
aborts.

Fix: return early from shutdown() when ray.is_initialized() is False.
The call is unreachable during normal use (Ray is alive), and the
placement groups are already gone with Ray when this branch fires.

Verified with the mopd-qwen3-1.7b-3n8g-megatron-pack nightly recipe
(job 13917529): exit 0, no core_worker_process.cc:88 CHECK in the
driver log, both metric checks (train/loss, token_mult_prob_error)
PASS.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhiyu Li <zhiyul@NVIDIA.com>
@ZhiyuLi-Nvidia
ZhiyuLi-Nvidia force-pushed the fix/async-grpo-shutdown-crash branch from 5dacef4 to c247a8f Compare July 14, 2026 04:35
@ZhiyuLi-Nvidia ZhiyuLi-Nvidia changed the title fix: register RayVirtualCluster.shutdown via atexit to prevent shutdown crash fix: skip RayVirtualCluster.shutdown when Ray already torn down Jul 14, 2026
@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 5dacef4..c247a8f

@ZhiyuLi-Nvidia

Copy link
Copy Markdown
Contributor Author

/ok to test c247a8f

@yuki-97
yuki-97 enabled auto-merge (squash) July 14, 2026 05:33
@yuki-97
yuki-97 merged commit 89f5cdb into main Jul 14, 2026
85 checks passed
@yuki-97
yuki-97 deleted the fix/async-grpo-shutdown-crash branch July 14, 2026 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) r0.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants