Skip to content

[MISC] replace c10::optional with std::optional#25602

Merged
vllm-bot merged 2 commits intovllm-project:mainfrom
842974287:replace-c10-optional
Sep 24, 2025
Merged

[MISC] replace c10::optional with std::optional#25602
vllm-bot merged 2 commits intovllm-project:mainfrom
842974287:replace-c10-optional

Conversation

@842974287
Copy link
Contributor

@842974287 842974287 commented Sep 24, 2025

Purpose

Replace the usage of c10::optional with std::optional as c10::optional is being deprecated.

Test Plan

CI

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: Shiyan Deng <dsy842974287@meta.com>
@mergify mergify bot added the rocm Related to AMD ROCm label Sep 24, 2025
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 correctly replaces the deprecated c10::optional with std::optional in ROCm-specific C++ files, which is a good modernization step. The changes in csrc/rocm/ops.h and csrc/rocm/skinny_gemms.cu are straightforward and correctly implemented. The other changes in the pull request are minor stylistic adjustments to comment indentation. Overall, this is a clean and beneficial refactoring.

Copy link
Collaborator

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Please lint the code.

Signed-off-by: Shiyan Deng <dsy842974287@meta.com>
@houseroad houseroad added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 24, 2025
@houseroad houseroad enabled auto-merge (squash) September 24, 2025 22:54
@vllm-bot vllm-bot merged commit 5c1e496 into vllm-project:main Sep 24, 2025
80 of 89 checks passed
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: Shiyan Deng <dsy842974287@meta.com>
Signed-off-by: yewentao256 <zhyanwentao@126.com>
choprahetarth pushed a commit to Tandemn-Labs/vllm that referenced this pull request Oct 11, 2025
Signed-off-by: Shiyan Deng <dsy842974287@meta.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: Shiyan Deng <dsy842974287@meta.com>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Signed-off-by: Shiyan Deng <dsy842974287@meta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants