Add StorageClient abstract type and GCS Client Implementation by Bslabe123 · Pull Request #61 · kubernetes-sigs/inference-perf

Bslabe123 · 2025-04-30T17:27:59Z

Fixes: #59

Example config.yml for uploading report to GCS:

load:
  type: constant
  stages:
  - rate: 1
    duration: 30
vllm:
  api: completion
  model_name: meta-llama/Llama-2-7b-hf
  url: {model-server-ip}:{model-server-port}
tokenizer:
  pretrained_model_name_or_path: meta-llama/Llama-2-7b-hf
  token: {your-token}
data:
  type: shareGPT
storage:
  google_cloud_storage:
    path: path/to/reports
    report_file_prefix: label_
    bucket_name: {your-bucket}

Report destination will be logged:

Uploaded gs://{your-bucket}/path/to/reports/label_mock_report.json

Example of uploaded report:

{"total_requests": 33, "avg_prompt_tokens": 611.6363636363636, "avg_output_tokens": 28.636363636363637, "avg_time_per_request": 1.935312055128937}

GCS Screenshot:

jjk-g · 2025-05-01T16:18:15Z

+    def push_report(self, reports: List[str]) -> None:
+        for index, content in enumerate(reports):
+            filename = f"report-{index}"
+            blob_path = f"{self.path}/{filename}" if self.path else filename


We should make self.path optionally configurable instead of only base class reports-timestamp. Or at least the ability to specify a prefix so reports are written as bucket/prefix/reports-timestamp/reports-index

Made path and report_file_prefix configurable, added more about usage in the description

jjk-g · 2025-05-01T16:24:59Z

+    google_cloud_storage: Optional[GoogleCloudStorageConfig] = None
+
+    @model_validator(mode="after")
+    def check_oneof(self) -> "ReportStorageConfig":


It may make sense to allow multiple ReportStorageConfig like local path and GCS

In that case removed the check altogether

SachinVarghese

Looks good overall! Minor changes requested.

SachinVarghese · 2025-05-02T17:09:46Z

+        self.path = f"reports-{timestamp}"
+        print(f"Reports will be stored at: {self.path}")
+
+    def push_report(self, reports: List[str]) -> None:


save is a more generic term with storage solutions and appropriate for the abstract class.

Further, we should allow reportgen to pass a list of fileObjects (with filename and content). The storage client should be agnostic of reporting process and should only deal with storing of such file objects at the path.

Suggested change

def push_report(self, reports: List[str]) -> None:

def save_report(self, reports: List[ReportFile]) -> None:

Done, also heads up moved storage from config.report to config

SachinVarghese · 2025-05-02T17:21:49Z

+        return self
+
+
 class ReportConfig(BaseModel):


can we add an enum type for the report - defaulting to mock report?

This is being addressed in a follow up PR, will link once its reviewable

and revert unneeded changes in base.py deduplication missing licence statement in gcs.py deduplicate push_report tweak if/else in main.py addressing requested changes add ReportFile to init move reportfile remove duplciate import remove unneeded import move reportfile remove storageclient from reportgen to fix circular dependency remove hanging import lint change import change import fix imports reorder imports add missing __init__.py fix circular import remove unneeded import reportfile_prefix optional storage_clietns = [] missing positional argument missing positional argument ReportFile should not be BaseModel incorrect field mame update get_contents() remove reference to fileobj should loop over storage clients debug fix missing .get_contents() call model.dumps -> model_dump remove indent fix _store_locally fix upload_from_string types move storage to root of config config.report.storage -> config.storage typo reportfile_prefix -> report_file_prefix lint remove config from storage __init__ check if file already exists

achandrasekar · 2025-05-06T17:47:50Z

Can you run make validate and post the output here so we can make sure lint checks pass before merging?

Bslabe123 · 2025-05-06T18:54:31Z

Can you run make validate and post the output here so we can make sure lint checks pass before merging?

Successfully built inference-perf
Installing collected packages: inference-perf
  Attempting uninstall: inference-perf
    Found existing installation: inference-perf 0.0.1
    Uninstalling inference-perf-0.0.1:
      Successfully uninstalled inference-perf-0.0.1
Successfully installed inference-perf-0.0.1
Formatting Python files with ruff format...
.venv/bin/ruff format
28 files left unchanged
Linting Python files with ruff check...
.venv/bin/ruff check
All checks passed!
Running type checking with mypy...
.venv/bin/mypy --strict ./inference_perf
Success: no issues found in 26 source files

achandrasekar · 2025-05-06T22:56:59Z

/lgtm
/approve

k8s-ci-robot · 2025-05-06T22:57:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achandrasekar, Bslabe123

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [achandrasekar]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 30, 2025

k8s-ci-robot requested review from SergeyKanzhelev and terrytangyuan April 30, 2025 17:28

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 30, 2025

jjk-g reviewed May 1, 2025

View reviewed changes

SachinVarghese requested changes May 2, 2025

View reviewed changes

Bslabe123 force-pushed the storage-client branch 3 times, most recently from 86c041e to dfc02e1 Compare May 5, 2025 21:05

Bslabe123 force-pushed the storage-client branch from 84dd69c to 34f20df Compare May 5, 2025 21:29

Bslabe123 requested review from SachinVarghese and jjk-g May 5, 2025 21:31

Bslabe123 added 2 commits May 5, 2025 21:47

remove MockReportGen

fc97149

remove super call

3acb8ca

nits

55a1327

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2025

Merge branch 'main' into storage-client

67f957c

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2025

lint

d7ded76

Bslabe123 mentioned this pull request May 6, 2025

[WIP] Report Generation #67

Closed

k8s-ci-robot assigned achandrasekar May 6, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2025

k8s-ci-robot merged commit 4551420 into kubernetes-sigs:main May 6, 2025
2 checks passed

Bslabe123 deleted the storage-client branch May 6, 2025 23:03

	def push_report(self, reports: List[str]) -> None:
	def save_report(self, reports: List[ReportFile]) -> None:

Conversation

Bslabe123 commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjk-g May 1, 2025

Choose a reason for hiding this comment

Uh oh!

Bslabe123 May 5, 2025

Choose a reason for hiding this comment

Uh oh!

jjk-g May 1, 2025

Choose a reason for hiding this comment

Uh oh!

Bslabe123 May 5, 2025

Choose a reason for hiding this comment

Uh oh!

SachinVarghese left a comment

Choose a reason for hiding this comment

Uh oh!

SachinVarghese May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Bslabe123 May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SachinVarghese May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Bslabe123 May 5, 2025

Choose a reason for hiding this comment

Uh oh!

achandrasekar commented May 6, 2025

Uh oh!

Bslabe123 commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achandrasekar commented May 6, 2025

Uh oh!

k8s-ci-robot commented May 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Bslabe123 commented Apr 30, 2025 •

edited

Loading

Bslabe123 May 5, 2025 •

edited

Loading

Bslabe123 commented May 6, 2025 •

edited

Loading