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

[Bugfix] update neuron for version > 0.5.0 #7175

Merged
merged 5 commits into from
Aug 15, 2024
Merged

[Bugfix] update neuron for version > 0.5.0 #7175

merged 5 commits into from
Aug 15, 2024

Conversation

omrishiv
Copy link
Contributor

@omrishiv omrishiv commented Aug 5, 2024

FILL IN THE PR DESCRIPTION HERE

FIX #6269

This is the first of multiple PRs to address some neuron issues.

vLLM >= 0.5.1 refactored the WorkerBase around. This adds the missing abstract method execute_worker that was failing, while also addressing a few other inconsistencies. This PR also expands the block-size choices due to neuron needing block-size = sequence-length. At the moment, tensor parallelism/model parallelism are not supported. That will come in the next PR.

@omrishiv
update neuron for 0.5.1
caf5032 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 5, 2024

👋 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 consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

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

🚀

@omrishiv
format
bf47bc6 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
@youkaichao youkaichao requested a review from liangfu August 6, 2024 00:23
@njhill njhill added the aws-neuron Related to AWS Inferentia & Trainium label Aug 6, 2024
liangfu
liangfu approved these changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@liangfu liangfu 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 @omrishiv for fixing NeuronWorker issue. The proposed fix looks good.

Would you rebase the latest main branch (, which tests against neuron sdk 2.19 since #6832), and help triage the CI/CD issue as well?

@omrishiv
Merge branch 'main' into neuron-6269-pt-1
1a621fa 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
@omrishiv
Copy link
Contributor Author

omrishiv commented Aug 14, 2024 •
edited
Loading

@liangfu I merged in main and am now failing tests, is this the CI/CD issue you mean?
Edit: apparently I hadn't properly merged in main. Now I'm not seeing the neuron test issue

@omrishiv
Merge branch 'main' into neuron-6269-pt-1
440055e 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
@simon-mo simon-mo merged commit 9c1f78d into vllm-project:main Aug 15, 2024
28 checks passed
@omrishiv omrishiv mentioned this pull request Aug 15, 2024
Merged
@omrishiv omrishiv deleted the neuron-6269-pt-1 branch August 15, 2024 17:33
kylesayrs pushed a commit to neuralmagic/vllm that referenced this pull request Aug 17, 2024
@omrishiv @DarkLight1337 @kylesayrs
[Bugfix] update neuron for version > 0.5.0 (vllm-project#7175)
c1402c3 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
Co-authored-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
fialhocoelho pushed a commit to opendatahub-io/vllm that referenced this pull request Aug 22, 2024
@omrishiv @DarkLight1337 @fialhocoelho
[Bugfix] update neuron for version > 0.5.0 (vllm-project#7175)
290c598 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
Co-authored-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
congcongchen123
congcongchen123 reviewed Aug 28, 2024
View reviewed changes
vllm/engine/arg_utils.py
@@ -293,7 +293,7 @@ def add_cli_args(parser: FlexibleArgumentParser) -> FlexibleArgumentParser:
parser.add_argument('--block-size',
type=int,
default=EngineArgs.block_size,
choices=[8, 16, 32],
choices=[8, 16, 32, 128, 256, 512, 1024, 2048],
Copy link

Choose a reason for hiding this comment

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

I am curious why we skip 64? Is it for a specific reason? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may have been an oversight during testing, but it's been addressed by #7562

Alvant pushed a commit to compressa-ai/vllm that referenced this pull request Oct 26, 2024
@omrishiv @DarkLight1337 @Alvant
[Bugfix] update neuron for version > 0.5.0 (vllm-project#7175)
13bfb24 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
Co-authored-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
Signed-off-by: Alvant <alvasian@yandex.ru>
KuntaiDu pushed a commit to KuntaiDu/vllm that referenced this pull request Nov 20, 2024
@omrishiv @DarkLight1337
[Bugfix] update neuron for version > 0.5.0 (vllm-project#7175)
e8a5fd7 
Signed-off-by: omrishiv <327609+omrishiv@users.noreply.github.com>
Co-authored-by: Cyrus Leung <cyrus.tl.leung@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@congcongchen123 congcongchen123 congcongchen123 left review comments

@liangfu liangfu liangfu approved these changes

Assignees
No one assigned
Labels
aws-neuron Related to AWS Inferentia & Trainium
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

f[Bug]: TypeError: Can't instantiate abstract class NeuronWorker with abstract method execute_worker
6 participants
@omrishiv @liangfu @congcongchen123 @njhill @simon-mo @DarkLight1337