Skip to content

Conversation

@JustinDuy
Copy link
Contributor

@JustinDuy JustinDuy commented Feb 27, 2025

FIX #150

BEFORE SUBMITTING, PLEASE READ THE CHECKLIST BELOW AND FILL IN THE DESCRIPTION ABOVE


  • Make sure the code changes pass the pre-commit checks.
  • Sign-off your commit by using -s when doing git commit
  • Try to classify PRs for easy understanding of the type of changes, such as [Bugfix], [Feat], and [CI].
Detailed Checklist (Click to Expand)

Thank you for your contribution to production-stack! Before submitting the pull request, please ensure the PR meets the following criteria. This helps us maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Please try to classify PRs for easy understanding of the type of changes. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

  • [Bugfix] for bug fixes.
  • [CI/Build] for build or continuous integration improvements.
  • [Doc] for documentation fixes and improvements.
  • [Feat] for new features in the cluster (e.g., autoscaling, disaggregated prefill, etc.).
  • [Router] for changes to the vllm_router (e.g., routing algorithm, router observability, etc.).
  • [Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

  • Pass all linter checks. Please use pre-commit to format your code. See README.md for installation.
  • The code need to be well-documented to ensure future contributors can easily understand the code.
  • Please include sufficient tests to ensure the change is stay correct and robust. This includes both unit tests and integration tests.

DCO and Signed-off-by

When contributing changes to this project, you must agree to the DCO. Commits must include a Signed-off-by: header which certifies agreement with the terms of the DCO.

Using -s with git commit will automatically add this header.

What to Expect for the Reviews

Could you please review this @YuhanLiu11 , @ggaaooppeenngg ?

@YuhanLiu11
Copy link
Collaborator

Hi @JustinDuy thanks for your contribution! Can you fix the DCO issue? (when committing, add -s in git commit).

Thanks a lot!

@ApostaC
Copy link
Collaborator

ApostaC commented Feb 28, 2025

@JustinDuy Thanks for contributing and it's great that you added functionality tests as well.

Seems that .github/values-05-secure-vllm.yaml is missing in this PR, can you also quickly fix it to make the functionality test pass?

@JustinDuy JustinDuy force-pushed the add-vllm-api-key branch 8 times, most recently from f339947 to 1c9c691 Compare March 5, 2025 11:10
@JustinDuy
Copy link
Contributor Author

@JustinDuy Thanks for contributing and it's great that you added functionality tests as well.

Seems that .github/values-05-secure-vllm.yaml is missing in this PR, can you also quickly fix it to make the functionality test pass?

@JustinDuy JustinDuy closed this Mar 5, 2025
@JustinDuy
Copy link
Contributor Author

@YuhanLiu11 , @ApostaC : i closed the PR by accident. Is it possible to re-open it?

@ApostaC ApostaC reopened this Mar 5, 2025
@kinoute
Copy link
Contributor

kinoute commented Mar 7, 2025

Great addition!

@JustinDuy
Copy link
Contributor Author

@YuhanLiu11 , @ApostaC , @Shaoting-Feng , @gaocegege : Could anyone trigger the test and review please?

ApostaC
ApostaC previously approved these changes Mar 7, 2025
Copy link
Collaborator

@ApostaC ApostaC left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change the file name since there are multiple new tutorials added recently. 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.

@ApostaC : Thank you! 've renamed the tutorial files. It's great to see the project advancing fast!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ApostaC : Could you please trigger the tests and merge if everything is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ApostaC : One more time please!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@JustinDuy just started running the tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YuhanLiu11, @ApostaC : Could you please trigger again? I increased the time-out. Look like we have this timeout error once in a while. Maybe timeout 2 mins should be increased also for the other helm tests...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@JustinDuy tests passed! I'll merge it now.

@JustinDuy JustinDuy force-pushed the add-vllm-api-key branch 4 times, most recently from a24705e to cc1f91a Compare March 10, 2025 22:37
@YuhanLiu11
Copy link
Collaborator

@JustinDuy seems to be some issues with the new test you added. 😢 Could you fix it when you get a chance? Thanks for your contribution!

Signed-off-by: kbvd623 <duy.nguyen4@astrazeneca.com>
Copy link
Collaborator

@YuhanLiu11 YuhanLiu11 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@YuhanLiu11 YuhanLiu11 merged commit 3793f97 into vllm-project:main Mar 11, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Model not found when enable vllm api key

4 participants