-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
[Doc] Fix UTF-8 encoding issues in documentation generation on Windows #24361
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.
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
[Doc] Fix UTF-8 encoding issues in documentation generation on Windows #24361
Conversation
There was a problem hiding this 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 addresses UTF-8 encoding issues for documentation generation on Windows. The changes are well-implemented and include new documentation and tests.
However, this PR critically mixes unrelated concerns by bundling the documentation fix with a significant, unrelated feature: adding sliding window attention support to FlexAttention. This includes changes to vllm/v1/attention/backends/flex_attention.py and new test files.
Mixed-concern pull requests complicate reviews and harm the project's long-term maintainability by polluting the git history. Please split this into two separate PRs: one for the documentation fix and one for the FlexAttention feature. I cannot approve this PR in its current state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function and other related changes for FlexAttention sliding window support are unrelated to this pull request's stated goal of fixing documentation encoding issues. Please move these changes, along with the corresponding tests, to a separate pull request. Mixing unrelated features in a single PR is a critical issue for repository maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmellor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix which makes the docs build more flexible!
However, I don't think we need the doc or the tests.
vLLM is not supported on Windows (only via WSL), so development instructions for Windows itself might give the wrong idea.
As for the tests, a comment next to the file opening lines saying # Specify encoding for building on Windows is sufficient.
0846e4d to
797ce24
Compare
|
@hmellor Thank you for the feedback! You're absolutely right - I've removed the Windows documentation The PR now contains only:
|
hmellor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating!
|
Please fix DCO then I'll be able to merge |
0e1529f to
2ff9efb
Compare
Fixes vllm-project#24358: FlexAttention does not support sliding window yet This commit implements sliding window attention support for the FlexAttention backend, enabling models like GptOss to work with FlexAttention when FlashAttention v2 is unavailable (e.g., on GPUs with compute capability < 8.0). Key changes: - Add sliding_window_causal_mask_mod() function for sliding window masks - Update FlexAttentionImpl to handle sliding window configuration - Enhance FlexAttentionMetadata with sliding_window field - Update FlexAttentionMetadataBuilder to extract sliding window from config - Add comprehensive unit and integration tests The implementation maintains backward compatibility and has no performance impact on models without sliding window. Tested with: - Unit tests for mask functions and metadata handling - Integration tests with FlexAttentionImpl - Edge cases and error conditions - Backward compatibility verification Before this change: - Models with sliding window would fail with NotImplementedError - Users had to manually force different attention backends - GptOss models couldn't use FlexAttention on older GPUs After this change: - FlexAttention supports sliding window attention seamlessly - GptOss models work out of the box with FlexAttention - Maintains full backward compatibility for models without sliding window Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com>
Fixes vllm-project#24360: Cannot build documentation locally without adding encoding This commit resolves Windows-specific encoding issues when building documentation locally by explicitly specifying UTF-8 encoding for file operations in documentation generation scripts. Changes: - Add encoding='utf-8' to file operations in generate_examples.py - Add encoding='utf-8' to file operations in generate_argparse.py - Add comprehensive tests for Unicode handling - Add Windows development guide with encoding troubleshooting Root Cause: Windows uses cp1252 (Windows-1252) as default encoding, which cannot handle Unicode characters like \uff5c (fullwidth vertical bar |). This caused UnicodeEncodeError when processing files with Unicode content. Solution: Explicitly specify UTF-8 encoding for all file read/write operations in documentation generation hooks, ensuring consistent behavior across all platforms. Testing: - Added unit tests for Unicode character handling - Tested with problematic characters from the original error - Verified backward compatibility with ASCII-only content - Added edge case testing for empty files and missing titles Benefits: - Enables local documentation building on Windows - Improves cross-platform development experience - Prevents similar encoding issues in the future - Maintains full backward compatibility Before this change: - Documentation build failed on Windows with UnicodeEncodeError - Contributors on Windows couldn't build docs locally - Unicode characters in examples caused build failures After this change: - Documentation builds successfully on Windows - Full Unicode support in documentation content - Consistent behavior across all platforms Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com>
This commit removes FlexAttention sliding window support changes that were incorrectly included in the documentation encoding fix PR. These changes belong in a separate PR to maintain clean commit history and proper separation of concerns. Removed: - FlexAttention sliding window implementation - Related test files for sliding window functionality - Unrelated changes to flex_attention.py This PR should now focus solely on fixing UTF-8 encoding issues in documentation generation scripts as originally intended. Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com>
Specify encoding explicitly in file operations to prevent UnicodeEncodeError when building docs on Windows systems that default to cp1252 encoding. Fixes vllm-project#24360 Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com>
2ff9efb to
c558860
Compare
|
@hmellor DCO is now fixed! All commits have been properly signed off with the correct email address. Thanks again! |
vllm-project#24361) Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Co-authored-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com>
vllm-project#24361) Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Co-authored-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com>
vllm-project#24361) Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Co-authored-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com>
vllm-project#24361) Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Co-authored-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
vllm-project#24361) Signed-off-by: alekramelaheehridoy <aliqramalaheehridoy@gmail.com> Signed-off-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Co-authored-by: alekramelaheehridoy <alekramelaheehridoy@gmail.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Summary
Fixes #24360: Cannot build documentation locally without adding encoding to generate examples
This PR resolves Windows-specific encoding issues when building documentation locally by explicitly specifying UTF-8 encoding for file operations in documentation generation scripts.
Problem
On Windows, Python's default file encoding is often
cp1252(Windows-1252), which cannot handle Unicode characters like\uff5c(fullwidth vertical bar|). This caused:Error Examples
Solution
Core Changes
docs/mkdocs/hooks/generate_examples.py: Addedencoding='utf-8'to file operationsdocs/mkdocs/hooks/generate_argparse.py: Addedencoding='utf-8'to file operationsFiles Modified