Skip to content

Conversation

OYE93
Copy link
Contributor

@OYE93 OYE93 commented Aug 25, 2025

Purpose

To fix the Installation for Apple Silicon (CPU), when following the doc, I met the error like

/vllm/csrc/cpu/dnnl_helper.cpp:89:8: error: no template
      named 'optional' in namespace 'std'
         89 |   std::optional<value_iterator_t> get_value(const KT& key) {
            |   ~~~~~^

After some checking, the cause could be that std::optional is a C++17 feature. So in this PR, just changed to force the C++17 compilation in the .cmake file.

Test Plan

uv pip install -e .

Test Result

After

Resolved 137 packages in 6.54s
      Built vllm @ file:///XXXXX/vllm
Prepared 1 package in 1m 42s
Installed 1 package in 2ms
 + vllm==0.1.dev8866+g92687fbe1.cpu (from file:///XXXXXX/vllm)

(Optional) Documentation Update


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.

@OYE93 OYE93 requested a review from bigPYJ1151 as a code owner August 25, 2025 15:32
@mergify mergify bot added the ci/build label Aug 25, 2025
@OYE93 OYE93 changed the title [Hardware][Mac] Fix the installation fail in Apple Silicon (CPU) [Hardware][Mac] Fix the installation fail for Apple Silicon (CPU) Aug 25, 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 identifies and fixes a build failure on Apple Silicon by enforcing the C++17 standard for CPU builds. The use of std::optional in the C++ source code makes this change necessary. The proposed fix is effective. I have one suggestion regarding CMake best practices to improve the long-term maintainability of the build configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

high

While setting CMAKE_CXX_STANDARD is a valid way to enable C++17 and fixes the build, it's a directory-level setting. A more modern and maintainable approach in CMake is to specify C++ standard requirements on a per-target basis using target_compile_features. This improves modularity and makes the build configuration easier to maintain, as target requirements are co-located with the target definition.

For example:

# After the target is defined around line 300
target_compile_features(_C PRIVATE cxx_std_17)

This would make the target's requirements explicit. Since this change works and its scope is limited to CPU builds, this is a suggestion for better long-term maintenance of the build system.

Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors.

You ask your reviewers to trigger select CI tests on top of fastcheck CI.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

🚀

@OYE93 OYE93 force-pushed the fix/cpu-installation branch from 92687fb to 209e463 Compare August 25, 2025 15:36
Copy link
Member

@bigPYJ1151 bigPYJ1151 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!

@bigPYJ1151 bigPYJ1151 enabled auto-merge (squash) August 26, 2025 02:25
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 26, 2025
@bigPYJ1151 bigPYJ1151 merged commit b78bed1 into vllm-project:main Aug 26, 2025
70 checks passed
tc-mb pushed a commit to tc-mb/vllm that referenced this pull request Aug 27, 2025
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 28, 2025
xiao-llm pushed a commit to xiao-llm/vllm that referenced this pull request Aug 28, 2025
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Aug 28, 2025
zhewenl pushed a commit to zhewenl/vllm that referenced this pull request Sep 3, 2025
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants