Skip to content

Conversation

@raulchen
Copy link
Contributor

Some SkyRL dependencies are only available on CUDA.
This PR adds conditions for these dependencies.
So uv sync --active --extra vllm would work on macOS.
Note, we still cannot run SkyRL on macOS, unless we make the imports optional.
But it would make local development easier by allowing navigating the codebase.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to make CUDA-specific dependencies optional on non-Linux systems. While it correctly adds platform markers for several packages, it misses some key Linux-only dependencies like vllm, sglang, and transformer-engine. Without these changes, the goal of enabling dependency installation on macOS will not be fully achieved. I've added critical comments to address these omissions. I also have a suggestion to improve the maintainability of the configuration.

"litellm[proxy]>=1.67.5",
]
vllm = [
"vllm==0.11.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The vllm package is only available for Linux. To achieve the goal of this PR and allow installation on other platforms like macOS, you should add a platform marker to this dependency as well.

    "vllm==0.11.0; sys_platform == 'linux'",

"torchvision"
]
sglang = [
"sglang[srt,openai,torch_memory_saver]==0.4.8.post1", # 0.4.9.post1 causes non-colocate weight broadcast to hang
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The sglang package is only available for Linux. To allow installation on other platforms, you should add a platform marker to this dependency.

    "sglang[srt,openai,torch_memory_saver]==0.4.8.post1; sys_platform == 'linux'",

Comment on lines 142 to 144
"transformer-engine[pytorch]==2.7.0",
"flash-attn==2.7.4.post1",
"flash-attn==2.7.4.post1; sys_platform == 'linux'",
"vllm==0.10.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The transformer-engine and vllm packages are Linux-only. To ensure the dependencies can be resolved on non-Linux systems, you should add platform markers to them.

  "transformer-engine[pytorch]==2.7.0; sys_platform == 'linux'",
  "flash-attn==2.7.4.post1; sys_platform == 'linux'",
  "vllm==0.10.1.1; sys_platform == 'linux'",

Comment on lines +98 to +99
{ url = "https://download.pytorch.org/whl/cu128/flashinfer/flashinfer_python-0.2.6.post1%2Bcu128torch2.7-cp39-abi3-linux_x86_64.whl", marker = "extra == 'mcore' and extra != 'vllm' and sys_platform == 'linux'" },
{ url = "https://download.pytorch.org/whl/cu128/flashinfer/flashinfer_python-0.2.6.post1%2Bcu128torch2.7-cp39-abi3-linux_x86_64.whl", marker = "extra == 'sglang' and extra != 'mcore' and extra != 'vllm' and sys_platform == 'linux'" }
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For better readability and maintainability, you can combine these two flashinfer-python source definitions into a single entry. Since they point to the same URL and the mcore and sglang extras are mutually exclusive according to your [tool.uv].conflicts configuration, their markers can be simplified and merged.

    { url = "https://download.pytorch.org/whl/cu128/flashinfer/flashinfer_python-0.2.6.post1%2Bcu128torch2.7-cp39-abi3-linux_x86_64.whl", marker = "(extra == 'mcore' or extra == 'sglang') and sys_platform == 'linux'" }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants