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

Add instructions to run benchmarks #480

Merged
merged 5 commits into from
Mar 18, 2025
Merged

Add instructions to run benchmarks #480

merged 5 commits into from
Mar 18, 2025

Conversation

liu-cong
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2025
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 12, 2025
@netlify Netlify
Copy link

netlify bot commented Mar 12, 2025 •
edited
Loading

✅ Deploy Preview for gateway-api-inference-extension ready!

Name Link
🔨 Latest commit dda25ac
🔍 Latest deploy log https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67d9e5d24684eb00087b4f0b
😎 Deploy Preview https://deploy-preview-480--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@liu-cong
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2025
ahg-g
ahg-g reviewed Mar 12, 2025
View reviewed changes
ahg-g
ahg-g reviewed Mar 13, 2025
View reviewed changes
benchmark/manifests/BenchmarkInferenceExtension.yaml Outdated
@@ -0,0 +1,60 @@
apiVersion: apps/v1
kind: Deployment
Copy link
Contributor

Choose a reason for hiding this comment

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

@achandrasekar How would one start another run? Should we use a Job here instead, something that runs to completion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this as well. A deployment is convenient in that it keeps the pods running so we can download the result files from the pod, otherwise we need to set up some persistent storage such as s3 or GCS, not every user has access to those. This is also aligns with the user guide of the lpg tool.

Copy link

Choose a reason for hiding this comment

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

You can give users option to export the result to s3 or GCS in the job.

Copy link

Choose a reason for hiding this comment

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

I think the pod/job/files stays around after it completes, so should still be able to d/l the results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can give users option to export the result to s3 or GCS in the job.

I took the approach that requires minimal dependencies. Yes using a persistent volumes such as S3 works as well, but it requires additional configuration. We can add that option later.

I think the pod/job/files stays around after it completes, so should still be able to d/l the results?

You will need some persistent volume.

I updated the download-benchmark-result.sh script to tear down the deployment after it downloads the results.

@ahg-g
Copy link
Contributor

ahg-g commented Mar 13, 2025

This worked for me, thanks!

A couple of things to improve:

  1. How to trigger multiple runs?
  2. How would the user know that the benchmark finished?

@danehans danehans mentioned this pull request Mar 13, 2025
Open
@ahg-g
Copy link
Contributor

ahg-g commented Mar 13, 2025

A couple more things:

  1. what is the plan for the image?
  2. where can the user learn about the config options for the benchmark?

ahg-g
ahg-g reviewed Mar 13, 2025
View reviewed changes
Copy link
Contributor

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

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

We can now link to the configuration options: https://github.com/AI-Hypercomputer/inference-benchmark?tab=readme-ov-file#configuring-the-benchmark

@kfswain kfswain self-requested a review March 14, 2025 20:25
@liu-cong
Copy link
Contributor Author

How to trigger multiple runs?

You need to re-deploy the benchmark tool. Note further orchestration of the tool such as automating multiple runs is out of the scope for this PR. Though we can revisit.

How would the user know that the benchmark finished?

Updated README

@kfswain
Copy link
Collaborator

kfswain commented Mar 17, 2025

Benchmark works for me! Thanks!

Some smaller items to consider:

  • We may want to move this README to be part of site-src/ so that the guide can be on our website, it may be odd to have guides in different places
  • moving any k8s manifests to config/manifests
  • snake casing the yaml files (we've let some kebab cased files in, but I think PascalCasing sticks out a tad more)

christian-posta
christian-posta reviewed Mar 18, 2025
View reviewed changes
Copy link

@christian-posta christian-posta left a comment

Choose a reason for hiding this comment

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

I've taken the files and run them. Seems to work. Left some comments. Thanks!

benchmark/manifests/BenchmarkInferenceExtension.yaml Outdated
@@ -0,0 +1,60 @@
apiVersion: apps/v1
kind: Deployment
Copy link

Choose a reason for hiding this comment

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

I think the pod/job/files stays around after it completes, so should still be able to d/l the results?

@liu-cong
Copy link
Contributor Author

Some smaller items to consider:

We may want to move this README to be part of site-src/ so that the guide can be on our website, it may be odd to have guides in different places
moving any k8s manifests to config/manifests
snake casing the yaml files (we've let some kebab cased files in, but I think PascalCasing sticks out a tad more)

Thanks! Updated as you suggested.

@ahg-g
Copy link
Contributor

ahg-g commented Mar 18, 2025

/approve

One nit to fix the link. However, this is still not very practical, we need a published helm chart so that users can run the benchmark without needing to fork.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, liu-cong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2025
@liu-cong liu-cong mentioned this pull request Mar 18, 2025
Open
@liu-cong
Copy link
Contributor Author

we need a published helm chart so that users can run the benchmark without needing to fork.

makes sense, created #528 to follow up on this.

@liu-cong
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2025
@ahg-g
Copy link
Contributor

ahg-g commented Mar 18, 2025

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2025
@k8s-ci-robot k8s-ci-robot merged commit 64ba0c6 into kubernetes-sigs:main Mar 18, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christian-posta christian-posta

@ahg-g ahg-g

@xzhaosg xzhaosg

@Jeffwan Jeffwan

@robscott robscott

@kfswain kfswain

Assignees

@ahg-g ahg-g

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liu-cong @ahg-g @kfswain @k8s-ci-robot @christian-posta @xzhaosg