-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
[Misc] update doc comment for send #22026
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
[Misc] update doc comment for send #22026
Conversation
Signed-off-by: Andy Xie <andy.xning@gmail.com>
|
👋 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 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 🚀 |
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 updates the docstrings for the send methods to indicate that they are blocking operations. My review focuses on improving the docstring format for better clarity and adherence to Python conventions. I've suggested combining the separate string literals into a single, multi-line docstring in all three affected files. This ensures that documentation generation tools will correctly parse the entire comment.
| """Sends a tensor to the destination rank in a blocking way""" | ||
| """NOTE: `dst` is the local rank of the destination rank.""" |
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.
While the change to 'blocking' is correct, the docstring format is unconventional and can be misleading. According to PEP 257, only the first string literal following a function definition is treated as its docstring. Subsequent string literals are simply statements that are evaluated and then discarded, having no effect.
To ensure the entire comment is recognized as the docstring by documentation tools and help(), it should be a single multi-line string.
| """Sends a tensor to the destination rank in a blocking way""" | |
| """NOTE: `dst` is the local rank of the destination rank.""" | |
| """Sends a tensor to the destination rank in a blocking way. | |
| NOTE: `dst` is the local rank of the destination rank. | |
| """ |
| """Sends a tensor to the destination rank in a blocking way""" | ||
| """NOTE: `dst` is the local rank of the destination rank.""" |
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.
While the change to 'blocking' is correct, the docstring format is unconventional and can be misleading. According to PEP 257, only the first string literal following a function definition is treated as its docstring. Subsequent string literals are simply statements that are evaluated and then discarded, having no effect.
To ensure the entire comment is recognized as the docstring by documentation tools and help(), it should be a single multi-line string.
| """Sends a tensor to the destination rank in a blocking way""" | |
| """NOTE: `dst` is the local rank of the destination rank.""" | |
| """Sends a tensor to the destination rank in a blocking way. | |
| NOTE: `dst` is the local rank of the destination rank. | |
| """ |
| """Sends a tensor to the destination rank in a blocking way""" | ||
| """NOTE: `dst` is the local rank of the destination rank.""" |
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.
While the change to 'blocking' is correct, the docstring format is unconventional and can be misleading. According to PEP 257, only the first string literal following a function definition is treated as its docstring. Subsequent string literals are simply statements that are evaluated and then discarded, having no effect.
To ensure the entire comment is recognized as the docstring by documentation tools and help(), it should be a single multi-line string.
| """Sends a tensor to the destination rank in a blocking way""" | |
| """NOTE: `dst` is the local rank of the destination rank.""" | |
| """Sends a tensor to the destination rank in a blocking way. | |
| NOTE: `dst` is the local rank of the destination rank. | |
| """ |
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: Andy Xie <andy.xning@gmail.com> Signed-off-by: Jinzhen Lin <linjinzhen@hotmail.com>
Signed-off-by: Andy Xie <andy.xning@gmail.com> Signed-off-by: Noam Gat <noamgat@gmail.com>
Signed-off-by: Andy Xie <andy.xning@gmail.com> Signed-off-by: Paul Pak <paulpak58@gmail.com>
Signed-off-by: Andy Xie <andy.xning@gmail.com> Signed-off-by: Diego-Castan <diego.castan@ibm.com>
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Signed-off-by: Andy Xie <andy.xning@gmail.com>
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
According to the
tensor.distributed.senddoc, send is a synchronous op.Test Plan
NA
Test Result
NA
(Optional) Documentation Update