-
Notifications
You must be signed in to change notification settings - Fork 51
feat: add a new dir called testdata in test/e2e #605
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @Wangzy455! It looks like this is your first PR to volcano-sh/kthena 🎉 |
Summary of ChangesHello @Wangzy455, 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 significantly enhances the end-to-end testing infrastructure by adding a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a testdata directory for end-to-end tests, centralizing test configurations. This is a good organizational improvement. My review of the newly added YAML files has identified a few areas for improvement. There are some inconsistencies in resource references, such as parentRefs in HTTPRoute and ModelRoute pointing to a non-existent Gateway. Some configurations use Docker images from private registries, which could be problematic for community contributors. I also found an incorrect environment variable definition, some configurations that could be made more readable, and a typo in a filename (ModelServer-ds1.5b-pd-disaggragation.yaml should be disaggregation). I've provided specific suggestions for these points in the comments.
| parentRefs: | ||
| - group: gateway.networking.k8s.io | ||
| kind: Gateway | ||
| name: default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| spec: | ||
| modelName: "deepseek-binding-gateway" | ||
| parentRefs: | ||
| - name: "default" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - name: HUGGING_FACE_HUB_TOKEN | ||
| value: $HUGGING_FACE_HUB_TOKEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - name: HUGGING_FACE_HUB_TOKEN | ||
| value: $HUGGING_FACE_HUB_TOKEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mountPath: /models | ||
| containers: | ||
| - name: prefill | ||
| image: kvcache-container-image-hb2-cn-beijing.cr.volces.com/aibrix/vllm-openai:v0.10.0-cu128-nixl-v0.4.1-lmcache-0.3.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Docker image kvcache-container-image-hb2-cn-beijing.cr.volces.com/aibrix/vllm-openai:v0.10.0-cu128-nixl-v0.4.1-lmcache-0.3.2 (also used on line 120) appears to be from a private or region-specific registry. This could make it difficult for other contributors to run these E2E tests. Consider using an image from a public, globally accessible registry.
| value: $HUGGING_FACE_HUB_TOKEN | ||
| volumeMounts: | ||
| - mountPath: /dev/shm | ||
| name: dshm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - "--kv-transfer-config" | ||
| - '{"kv_connector":"MooncakeConnectorV1","kv_buffer_device":"npu","kv_role":"kv_producer","kv_parallel_size":1,"kv_port":"20001","engine_id":"0","kv_rank":0,"kv_connector_module_path":"vllm_ascend.distributed.mooncake_connector","kv_connector_extra_config":{"prefill":{"dp_size":2,"tp_size":2},"decode":{"dp_size":2,"tp_size":2}}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test/e2e/testdata/redis/README.md
Outdated
| namespace: <your-namespace> # Same namespace as Kthena | ||
| type: Opaque | ||
| data: | ||
| REDIS_PASSWORD: "base64-encoded-password" # Use empty string "" for no password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment # Use empty string "" for no password could be slightly confusing. It's more accurate to say that for an empty password, the value should be an empty string "", which is the Base64 encoding of an empty string.
| REDIS_PASSWORD: "base64-encoded-password" # Use empty string "" for no password | |
| REDIS_PASSWORD: "base64-encoded-password" # Base64-encoded password. For no password, use an empty string "". |
47af0e5 to
6eae511
Compare
| @@ -0,0 +1,11 @@ | |||
| apiVersion: gateway.networking.k8s.io/v1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only add yamls used by E2E now, like: https://github.com/volcano-sh/kthena/blob/main/test/e2e/router/e2e_test.go#L101 .
Replace all those calls under https://github.com/volcano-sh/kthena/tree/main/test/e2e/router
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -0,0 +1,98 @@ | |||
| apiVersion: workload.serving.volcano.sh/v1alpha1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently only router related yamls need to be added,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get
b3cdf73 to
69fe97b
Compare
|
@Wangzy455 Update these calls https://github.com/volcano-sh/kthena/blob/main/test/e2e/router/e2e_test.go#L101 as well. All these calls under |
get |
|
@YaoZengzeng Sorry, I think I have added all the YAML files we need, including ModelRouteSimple.yaml. |
|
/ok-to-test |
|
nit: prefer test/e2e/router/testdata |
hzxuzhonghu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should make the tests loading these new yamls
ok i'll fix it |
test/e2e/router/e2e_test.go
Outdated
| // Deploy ModelRoute | ||
| t.Log("Deploying ModelRoute...") | ||
| modelRoute := utils.LoadYAMLFromFile[networkingv1alpha1.ModelRoute]("examples/kthena-router/ModelRouteSimple.yaml") | ||
| modelRoute := utils.LoadYAMLFromFile[networkingv1alpha1.ModelRoute]("test/e2e/router/testdata/ModelRouteSimple.yaml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The yamls are not in this dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get
|
Failure panic: Failed to read YAML file from project root: test/e2e/router/testdata/ModelRouteSimple.yaml (abs: /home/runner/work/kthena/kthena/test/e2e/router/testdata/ModelRouteSimple.yaml): open /home/runner/work/kthena/kthena/test/e2e/router/testdata/ModelRouteSimple.yaml: no such file or directory |
Signed-off-by: Wangzy <[email protected]>
Signed-off-by: Wangzy <[email protected]>
Signed-off-by: Wangzy <[email protected]>
Signed-off-by: Wangzy <[email protected]>
Signed-off-by: Wangzy <[email protected]>
|
@Wangzy455 you can run the tests in your local env and make sure they all pass. |
get |
|
Friendly ping @Wangzy455 Any update? |
Sorry I was a bit busy last week, I'll finish it as soon as possible this week. |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
add a new dir to save testdata
Which issue(s) this PR fixes:
Fixes #602
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?:
NONE