Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly implements the logic to expose the VLLM_NIXL_SIDE_CHANNEL_PORT from the environment variables to the container ports for both decode and prefill pods. The accompanying tests validate this new functionality. I've added two review comments with suggestions to improve code maintainability by replacing a magic string with a constant and refactoring duplicated test code.
| // container port for both decode and prefill pods. | ||
| var nixlPort int32 | ||
| for _, e := range ms.Spec.Engine.Env { | ||
| if e.Name == "VLLM_NIXL_SIDE_CHANNEL_PORT" { |
There was a problem hiding this comment.
The string literal "VLLM_NIXL_SIDE_CHANNEL_PORT" is a magic string. It's also used in the test file. To improve maintainability and avoid potential typos, it's recommended to define this as a package-level constant. For example: const vllmNixlSideChannelPortEnv = "VLLM_NIXL_SIDE_CHANNEL_PORT".
| It("should expose VLLM_NIXL_SIDE_CHANNEL_PORT on vLLM container for decode and prefill", func() { | ||
| ms := newTestModelService() | ||
| expected := int32(6123) | ||
| ms.Spec.Engine.Env = append(ms.Spec.Engine.Env, corev1.EnvVar{ | ||
| Name: "VLLM_NIXL_SIDE_CHANNEL_PORT", | ||
| Value: "6123", | ||
| }) | ||
|
|
||
| role := &ms.Spec.Decode | ||
|
|
||
| decodeDep := BuildDeployment(ms, role, RoleDecode, "test-decode", nil, nil, nil, TracingConfig{}, testKitImage) | ||
| decodeVLLM := decodeDep.Spec.Template.Spec.Containers[0] | ||
| var foundDecode bool | ||
| for _, p := range decodeVLLM.Ports { | ||
| if p.ContainerPort == expected && p.Protocol == corev1.ProtocolTCP { | ||
| foundDecode = true | ||
| break | ||
| } | ||
| } | ||
| Expect(foundDecode).To(BeTrue(), "Decode vLLM should expose VLLM_NIXL_SIDE_CHANNEL_PORT") | ||
|
|
||
| prefillDep := BuildDeployment(ms, role, RolePrefill, "test-prefill", nil, nil, nil, TracingConfig{}, testKitImage) | ||
| prefillVLLM := prefillDep.Spec.Template.Spec.Containers[0] | ||
| var foundPrefill bool | ||
| for _, p := range prefillVLLM.Ports { | ||
| if p.ContainerPort == expected && p.Protocol == corev1.ProtocolTCP { | ||
| foundPrefill = true | ||
| break | ||
| } | ||
| } | ||
| Expect(foundPrefill).To(BeTrue(), "Prefill vLLM should expose VLLM_NIXL_SIDE_CHANNEL_PORT") | ||
| }) |
There was a problem hiding this comment.
This test case contains duplicated logic for checking the 'decode' and 'prefill' roles. To make the test more concise and maintainable, consider refactoring it to use Ginkgo's DescribeTable to test both roles with a single, data-driven test implementation.
DescribeTable("should expose VLLM_NIXL_SIDE_CHANNEL_PORT on vLLM container for decode and prefill",
func(roleType, roleName, roleDisplayName string) {
ms := newTestModelService()
expected := int32(6123)
ms.Spec.Engine.Env = append(ms.Spec.Engine.Env, corev1.EnvVar{
Name: "VLLM_NIXL_SIDE_CHANNEL_PORT",
Value: "6123",
})
role := &ms.Spec.Decode
dep := BuildDeployment(ms, role, roleType, "test-"+roleName, nil, nil, nil, TracingConfig{}, testKitImage)
vllmContainer := dep.Spec.Template.Spec.Containers[0]
var found bool
for _, p := range vllmContainer.Ports {
if p.ContainerPort == expected && p.Protocol == corev1.ProtocolTCP {
found = true
break
}
}
Expect(found).To(BeTrue(), "%s vLLM should expose VLLM_NIXL_SIDE_CHANNEL_PORT", roleDisplayName)
},
Entry("for decode role", RoleDecode, "decode", "Decode"),
Entry("for prefill role", RolePrefill, "prefill", "Prefill"),
)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
No description provided.