Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Hardware] [HPU]add mark_step for hpu #10239

Merged
merged 2 commits into from
Nov 17, 2024
Merged

[Hardware] [HPU]add mark_step for hpu #10239

merged 2 commits into from
Nov 17, 2024

Conversation

jikunshang
Copy link
Contributor

@jikunshang jikunshang commented Nov 12, 2024 •
edited by github-actions bot
Loading

This PR add mark_step after each model's decoder layer, which could benefit performance and not break origin model files.
mark_step doc

cc @kzawora-intel

@github-actions GitHub Actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.
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 can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@jikunshang
add mark_step for hpu
4cd3598 
Signed-off-by: Kunshang Ji <[email protected]>
@madamczykhabana
Copy link
Contributor

madamczykhabana commented Nov 12, 2024 •
edited
Loading

Hi @jikunshang . Could the same functionality be implemented using stock PT functionalities like forward hooks (https://pytorch.org/docs/stable/generated/torch.nn.Module.html#torch.nn.Module.register_forward_hook)? I guess we only need to call mark_step() and there's no other complex logic involved.

@jikunshang
address comments
7e1e8a0 
Signed-off-by: Kunshang Ji <[email protected]>
@jikunshang
Copy link
Contributor Author

Hi @jikunshang . Could the same functionality be implemented using stock PT functionalities like forward hooks (https://pytorch.org/docs/stable/generated/torch.nn.Module.html#torch.nn.Module.register_forward_hook)? I guess we only need to call mark_step() and there's no other complex logic involved.

nice catch, addressed your comments. please take a look again, thanks!

@WoosukKwon
Copy link
Collaborator

@madamczykhabana Please take a look. I will merge the PR once you approve it.

madamczykhabana
madamczykhabana approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@madamczykhabana madamczykhabana left a comment

Choose a reason for hiding this comment

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

LGTM

@jikunshang
Copy link
Contributor Author

@WoosukKwon Can we merge this now?

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) November 17, 2024 08:30
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 17, 2024
@DarkLight1337
Copy link
Member

Sorry for missing this!

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) November 17, 2024 08:30
@WoosukKwon WoosukKwon disabled auto-merge November 17, 2024 08:44
@WoosukKwon WoosukKwon merged commit 76aab90 into vllm-project:main Nov 17, 2024
36 of 50 checks passed
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Nov 18, 2024
@jikunshang
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
305708b 
Signed-off-by: Kunshang Ji <[email protected]>
Signed-off-by: Linkun Chen <[email protected]>
@jiminha jiminha mentioned this pull request Nov 20, 2024
Merged
coolkp pushed a commit to coolkp/vllm that referenced this pull request Nov 20, 2024
@jikunshang @coolkp
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
860bd51 
Signed-off-by: Kunshang Ji <[email protected]>
KuntaiDu pushed a commit to KuntaiDu/vllm that referenced this pull request Nov 20, 2024
@jikunshang
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
6d9f042 
Signed-off-by: Kunshang Ji <[email protected]>
mfournioux pushed a commit to mfournioux/vllm that referenced this pull request Nov 20, 2024
@jikunshang @mfournioux
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
ac3a0da 
Signed-off-by: Kunshang Ji <[email protected]>
Signed-off-by: Maxime Fournioux <[email protected]>
rickyyx pushed a commit to rickyyx/vllm that referenced this pull request Nov 20, 2024
@jikunshang @rickyyx
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
c44141f 
Signed-off-by: Kunshang Ji <[email protected]>
Signed-off-by: rickyx <[email protected]>
tlrmchlsmth pushed a commit to neuralmagic/vllm that referenced this pull request Nov 23, 2024
@jikunshang @tlrmchlsmth
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
d2a04af 
Signed-off-by: Kunshang Ji <[email protected]>
Signed-off-by: Tyler Michael Smith <[email protected]>
@jiminha jiminha mentioned this pull request Nov 26, 2024
Closed
michalkuligowski pushed a commit to HabanaAI/vllm-fork that referenced this pull request Nov 26, 2024
@jiminha
[HPU] Add mark_step configurable for the decoder layer. (#525)
b62f1b2 
We are seeing 10% performance regression in the llama-based model due to
vllm-project#10239. The mark_step()
function needs to be configured differently for each model to achieve
the best performance. For some models, mark_step() for every decoder
step would be optimal, but for other models, it's better to run it every
n-th step. We are adding a counter to only register the hook for every
n-th step, which can be configured with VLLM_CONFIG_HIDDEN_LAYERS
prashantgupta24 pushed a commit to opendatahub-io/vllm that referenced this pull request Dec 3, 2024
@jikunshang @prashantgupta24
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
f6dc8be 
Signed-off-by: Kunshang Ji <[email protected]>
sleepwalker2017 pushed a commit to sleepwalker2017/vllm that referenced this pull request Dec 13, 2024
@jikunshang
[Hardware] [HPU]add mark_step for hpu (vllm-project#10239)
5884cba 
Signed-off-by: Kunshang Ji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madamczykhabana madamczykhabana madamczykhabana approved these changes

@WoosukKwon WoosukKwon WoosukKwon approved these changes

Assignees
No one assigned
Labels
Gaudi ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jikunshang @madamczykhabana @WoosukKwon @DarkLight1337