Skip to content

Replace cuda_graph assertion with explicit ValueError; fix custom_op type annotations - #8336

Open
chakshu-dhannawat wants to merge 2 commits into
deepspeedai:masterfrom
chakshu-dhannawat:fix/cuda-graph-validation-8330
Open

Replace cuda_graph assertion with explicit ValueError; fix custom_op type annotations#8336
chakshu-dhannawat wants to merge 2 commits into
deepspeedai:masterfrom
chakshu-dhannawat:fix/cuda-graph-validation-8330

Conversation

@chakshu-dhannawat

Copy link
Copy Markdown

This PR addresses #8330 by replacing the internal assertion in replace_with_policy with a clear ValueError when both replace_with_kernel_inject=True and enable_cuda_graph=True are requested for a model whose policy does not support CUDA graphs.

Changes:

  • deepspeed/module_inject/replace_module.py: raise an actionable ValueError instead of asserting.
  • tests/unit/inference/test_inference_config.py: add a regression test using a tiny Llama model that verifies the new error message.
  • deepspeed/compile/custom_ops/tp_collectives.py: replace list[int] annotations with typing.List[int] so import deepspeed works on PyTorch 2.6+ (the new regression test needs this to import cleanly).

The regression test was verified on a single H100 with:

PYTHONPATH=tests/unit pytest tests/unit/inference/test_inference_config.py::TestInferenceCudaGraphConfig::test_cuda_graph_with_kernel_inject_raises -m inference -v

PyTorch's torch.library.infer_schema does not accept builtin list[int] annotations; use typing.List[int] instead so deepspeed imports cleanly on recent PyTorch releases.

Signed-off-by: Chakshu Dhannawat <chakshu.dhannawat1@gmail.com>
When replace_with_kernel_inject and enable_cuda_graph are both requested for a model whose policy does not support CUDA graphs, raise a clear ValueError instead of an internal assertion.

Fixes deepspeedai#8330

Signed-off-by: Chakshu Dhannawat <chakshu.dhannawat1@gmail.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e4cde7fc1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

intermediate_size=64,
torch_dtype=torch.bfloat16,
)
model = LlamaForCausalLM(model_config).to("cuda")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use the active accelerator in the new inference test

When this inference test is selected on a non-CUDA accelerator where get_accelerator().is_available() is true (for example XPU/HPU), the class does not skip, but moving the model with .to("cuda") fails before deepspeed.init_inference() can raise the ValueError being tested. Please either skip unless the selected accelerator is CUDA, or move the model to get_accelerator().device_name() so accelerator-specific test runs do not fail for an unrelated reason.

Useful? React with 👍 / 👎.

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.

1 participant