Skip to content

[Spyre-Next] 🎨 Fix docstring inaccuracies and typos#880

Merged
yannicks1 merged 17 commits intotorch-spyre:mainfrom
yannicks1:small_things
Apr 9, 2026
Merged

[Spyre-Next] 🎨 Fix docstring inaccuracies and typos#880
yannicks1 merged 17 commits intotorch-spyre:mainfrom
yannicks1:small_things

Conversation

@yannicks1
Copy link
Copy Markdown
Collaborator

Description

Fix docstring inaccuracies, typos and typing.

Changes:

Test Plan

Documentation-only changes, no functional impact.

Checklist

  • I have read the contributing guidelines
  • My code follows the project's code style (run bash format.sh)
  • I have added tests for my changes (if applicable)
  • I have updated the documentation (if applicable)
  • My commits include a Signed-off-by: line (DCO compliance)

Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
@github-actions github-actions bot changed the title 🎨 Fix docstring inaccuracies and typos [Spyre-Next] 🎨 Fix docstring inaccuracies and typos Mar 31, 2026

Key differences from upstream:
- Uses transpose(-1, -2) for computation efficiency on Spyre
- Creates epsilon tensor via torch.ops.spyre.full() instead of scalar
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@bohnstingl I was wondering: is this still true? Is full a custom spyre ops or do we use torch.full?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, this is not true anymore. We can now just use torch.full with torch-spyre.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I changed that with my latest PR

@@ -18,7 +18,6 @@
- Minimum batch size: 64 (due to spyre constraint, automatically padded)
- Device dtype: float16 (converted for CPU)
- Output dtype: bfloat16 (converted on CPU)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@bohnstingl Is this always bfloat16 or is it just matching the input data type?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually, maybe we can rephrase these dtypes a bit in general?
The Input dtype is defined by the model, respectively by the user. The computation in our wrappings are then always carried out in torch.float16.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

do you have a suggestion?

sth like

Suggested change
- Output dtype: bfloat16 (converted on CPU)
- Output dtype: model data type/ user defined (converted on CPU)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

what about the bfloat mentions in line 158 and 168?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Corrected in my PR.

@github-actions
Copy link
Copy Markdown

👋 Hi! Thank you for contributing to vLLM support on Spyre.
Just a reminder: Make sure that your code passes all the linting checks, otherwise your PR won't be able to be merged. To do so, run ./format.sh.
Now you are good to go 🚀.

We also recommend installing prek and configuring it to check your code before every local commit.

Copy link
Copy Markdown
Collaborator

@bohnstingl bohnstingl left a comment

Choose a reason for hiding this comment

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

Thank you very much @yannicks1 for opening this PR. I think it is very valuable to do these kind of refactors every once in a while 😊

@@ -18,7 +18,6 @@
- Minimum batch size: 64 (due to spyre constraint, automatically padded)
- Device dtype: float16 (converted for CPU)
- Output dtype: bfloat16 (converted on CPU)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually, maybe we can rephrase these dtypes a bit in general?
The Input dtype is defined by the model, respectively by the user. The computation in our wrappings are then always carried out in torch.float16.


Key differences from upstream:
- Uses transpose(-1, -2) for computation efficiency on Spyre
- Creates epsilon tensor via torch.ops.spyre.full() instead of scalar
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, this is not true anymore. We can now just use torch.full with torch-spyre.

Signed-off-by: Thomas Ortner <boh@zurich.ibm.com>
@bohnstingl
Copy link
Copy Markdown
Collaborator

@yannicks1, could you please rebase to latest main?
I also made some additional, small docstring changes. Please check and feel free to either merge in or ignore. yannicks1#1

[Docstrings] Some additional docstring updates
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
@@ -18,7 +18,6 @@
- Minimum batch size: 64 (due to spyre constraint, automatically padded)
- Device dtype: float16 (converted for CPU)
- Output dtype: bfloat16 (converted on CPU)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

do you have a suggestion?

sth like

Suggested change
- Output dtype: bfloat16 (converted on CPU)
- Output dtype: model data type/ user defined (converted on CPU)

@@ -18,7 +18,6 @@
- Minimum batch size: 64 (due to spyre constraint, automatically padded)
- Device dtype: float16 (converted for CPU)
- Output dtype: bfloat16 (converted on CPU)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

what about the bfloat mentions in line 158 and 168?

Comment thread vllm_spyre_next/vllm_spyre_next/custom_ops/rms_norm.py
Signed-off-by: Thomas Ortner <boh@zurich.ibm.com>
…ocstrings

Signed-off-by: Thomas Ortner <boh@zurich.ibm.com>
Copy link
Copy Markdown
Collaborator

@bohnstingl bohnstingl left a comment

Choose a reason for hiding this comment

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

@yannicks1 I created a PR with small changes again

Comment thread vllm_spyre_next/vllm_spyre_next/custom_ops/rms_norm.py
@@ -18,7 +18,6 @@
- Minimum batch size: 64 (due to spyre constraint, automatically padded)
- Device dtype: float16 (converted for CPU)
- Output dtype: bfloat16 (converted on CPU)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Corrected in my PR.


Key differences from upstream:
- Uses transpose(-1, -2) for computation efficiency on Spyre
- Creates epsilon tensor via torch.ops.spyre.full() instead of scalar
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I changed that with my latest PR

Added debug info about CustomOps
@yannicks1 yannicks1 enabled auto-merge (squash) April 9, 2026 08:15
Copy link
Copy Markdown
Collaborator

@bohnstingl bohnstingl left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions github-actions bot added the ready Runs the full CI test suite. Only add to PRs once ready to merge to limit public GHA usage label Apr 9, 2026
@yannicks1 yannicks1 requested a review from bohnstingl April 9, 2026 08:16
Signed-off-by: Yannick Schnider <Yannick.Schnider1@ibm.com>
@yannicks1 yannicks1 merged commit 95f6a44 into torch-spyre:main Apr 9, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready Runs the full CI test suite. Only add to PRs once ready to merge to limit public GHA usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants