Skip to content

Fix v1 supported oracle for worker-cls and worker-extension-cls #15324

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

Merged
merged 6 commits into from
Mar 23, 2025
Merged

Fix v1 supported oracle for worker-cls and worker-extension-cls #15324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
48a4da8
Fix arg_utils.py for OpenRLHF using vLLM V1 engine
hijkzzz Mar 22, 2025
effacb0
Update arg_utils.py
hijkzzz Mar 22, 2025
2d1e722
fix cluster init
youkaichao Mar 22, 2025
80626e5
add comments
youkaichao Mar 22, 2025
df8a73e
fix tests
youkaichao Mar 22, 2025
55075bf
update comments
youkaichao Mar 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .buildkite/test-pipeline.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ steps:
# TODO: create a dedicated test section for multi-GPU example tests
# when we have multiple distributed example tests
- pushd ../examples/offline_inference
- python3 rlhf.py
- RAY_DEDUP_LOGS=0 python3 rlhf_colocate.py
- VLLM_ENABLE_V1_MULTIPROCESSING=0 python3 rlhf.py
Copy link
Member

@youkaichao youkaichao Mar 22, 2025

Choose a reason for hiding this comment

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

in v1, LLM.collective_rpc is broken in the default case, because self.llm_engine.model_executor is in a different process.

cc @njhill @robertgshaw2-redhat if you can help fix it. i'm disabling VLLM_ENABLE_V1_MULTIPROCESSING for the test right now.

Copy link
Collaborator

@robertgshaw2-redhat robertgshaw2-redhat Mar 22, 2025

Choose a reason for hiding this comment

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

Do you think we should fall back to this automatically if the user selects --worker-cls?

Copy link
Member

@youkaichao youkaichao Mar 22, 2025

Choose a reason for hiding this comment

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

we don't need to fall back. worker-cls and rlhf can be separate things.

- VLLM_ENABLE_V1_MULTIPROCESSING=0 RAY_DEDUP_LOGS=0 python3 rlhf_colocate.py
- popd

- label: Metrics, Tracing Test # 10min
Expand Down
10 changes: 0 additions & 10 deletions vllm/engine/arg_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1460,16 +1460,6 @@ def _is_v1_supported_oracle(self, model_config: ModelConfig) -> bool:
recommend_to_remove=False)
return False

if self.worker_cls != EngineArgs.worker_cls:
_raise_or_fallback(feature_name="--worker-cls",
recommend_to_remove=False)
return False

if self.worker_extension_cls != EngineArgs.worker_extension_cls:
_raise_or_fallback(feature_name="--worker-extension-cls",
recommend_to_remove=False)
return False

Comment on lines -1463 to -1472
Copy link
Member

@youkaichao youkaichao Mar 22, 2025

Choose a reason for hiding this comment

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

cc @robertgshaw2-redhat for visibility.

if self.num_scheduler_steps != EngineArgs.num_scheduler_steps:
_raise_or_fallback(feature_name="--num-scheduler-steps",
recommend_to_remove=True)
Expand Down
5 changes: 5 additions & 0 deletions vllm/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2170,6 +2170,11 @@ def _maybe_force_spawn():
if cuda_is_initialized():
reason = "CUDA is initialized"
elif is_in_ray_actor():
# even if we choose to spawn, we need to pass the ray address
# to the subprocess so that it knows how to connect to the ray cluster.
# env vars are inherited by subprocesses, even if we use spawn.
import ray
Copy link
Collaborator

@ruisearch42 ruisearch42 Mar 23, 2025

Choose a reason for hiding this comment

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

ray is not always installed for vllm, we need a try-except like is_in_ray_actor()

Copy link
Member

@youkaichao youkaichao Mar 23, 2025

Choose a reason for hiding this comment

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

this is guarded under is_in_ray_actor(), so it is safe.

Copy link
Collaborator

@ruisearch42 ruisearch42 Mar 23, 2025

Choose a reason for hiding this comment

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

Yeah that's right, realized it afterwards

os.environ["RAY_ADDRESS"] = ray.get_runtime_context().gcs_address
reason = "In a Ray actor and can only be spawned"

if reason is not None:
Expand Down