Skip to content

feat: improve worker group args/kwargs #539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jul 1, 2025
Merged

feat: improve worker group args/kwargs #539

merged 5 commits into from
Jul 1, 2025

Conversation

yuki-97
Copy link
Contributor

@yuki-97 yuki-97 commented Jun 24, 2025

Under some conditions, ray stops respecting non-kwargs arguments.

This PR supports passing args/kwargs inrun_all_workers_multiple_data and run_all_workers_sharded_data, so that we can pass through kwargs instead of passing an unclear param data.

@yuki-97 yuki-97 added the CI:L1 Run doctests, unit tests, and functional tests label Jun 24, 2025
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Jun 24, 2025
@yuki-97 yuki-97 force-pushed the yukih/worker-group-args branch from 0da5ef1 to eef6d04 Compare June 24, 2025 11:08
@yuki-97 yuki-97 requested a review from SahilJain314 June 24, 2025 11:09
SahilJain314
SahilJain314 requested changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@SahilJain314 SahilJain314 left a comment

Choose a reason for hiding this comment

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

How should we handle the fact that passing args will fail under certain circumstances? Can we assert fail the args or remove them entirely for now? (until we figure out what's going on)? I don't want people to run into this as a sharp edge.

yuki-97 added 3 commits June 30, 2025 07:29
@yuki-97
support args/kwargs in run_all_workers_multiple_data and run_all_work…
3163a25 
…ers_sharded_data

Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97
add and revert args name
9dd9034 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97
update comment
98defde 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 force-pushed the yukih/worker-group-args branch from eef6d04 to 1eab584 Compare June 30, 2025 08:29
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Jun 30, 2025
@yuki-97
add assert and fix rebase
347d02c 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 force-pushed the yukih/worker-group-args branch from 1eab584 to 347d02c Compare June 30, 2025 08:31
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Jun 30, 2025
@yuki-97
fix unit test, add run_single_worker_single_data test
d5b6b52 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Jun 30, 2025
@yuki-97
Copy link
Contributor Author

yuki-97 commented Jun 30, 2025

How should we handle the fact that passing args will fail under certain circumstances? Can we assert fail the args or remove them entirely for now? (until we figure out what's going on)? I don't want people to run into this as a sharp edge.

@SahilJain314 Added assert. Also add an issue #582 to trace ray stops respecting non-kwargs arguments.

SahilJain314
SahilJain314 approved these changes Jun 30, 2025
View reviewed changes
Copy link
Contributor

@SahilJain314 SahilJain314 left a comment

Choose a reason for hiding this comment

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

assert fail LGTM for now. We can run the CI to make sure all instances are covered.

@SahilJain314 SahilJain314 added this pull request to the merge queue Jun 30, 2025
Merged via the queue into main with commit 2d79359 Jul 1, 2025
20 of 23 checks passed
@SahilJain314 SahilJain314 deleted the yukih/worker-group-args branch July 1, 2025 00:36
xxman-google pushed a commit to xxman-google/NeMo-RL that referenced this pull request Jul 2, 2025
@yuki-97 @xxman-google
feat: improve worker group args/kwargs (NVIDIA-NeMo#539)
658437d 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
therealnaveenkamal pushed a commit to therealnaveenkamal/RL that referenced this pull request Jul 7, 2025
@yuki-97 @therealnaveenkamal
feat: improve worker group args/kwargs (NVIDIA-NeMo#539)
a78fc15 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
YzjiaoNvd pushed a commit to YzjiaoNvd/NeMo-RL that referenced this pull request Jul 14, 2025
@yuki-97
feat: improve worker group args/kwargs (NVIDIA-NeMo#539)
e9d3ae8 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
KiddoZhu pushed a commit that referenced this pull request Jul 28, 2025
@yuki-97 @KiddoZhu
feat: improve worker group args/kwargs (#539)
af0430b 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SahilJain314 SahilJain314 SahilJain314 approved these changes

Assignees
No one assigned
Labels
CI:L1 Run doctests, unit tests, and functional tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yuki-97 @SahilJain314