Feat/mlperf_k8s_support - #124
Conversation
This commit adds comprehensive MLPerf inference harness integration for vLLM optimization: ## Core Changes: ### MLPerf Benchmark Provider (auto_tune_vllm/benchmarks/providers.py) - Implement full MLPerf result parsing from mlperf_log_summary.txt - Extract output_tokens_per_second and samples_per_second metrics - Add validation for required metrics - Support both Offline and Server scenarios ### Kubernetes Execution Backend (auto_tune_vllm/execution/helm_utils.py) - Add AWS credentials injection for MLflow S3 artifact uploads - AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_DEFAULT_REGION from k8s secret - Add HuggingFace environment variables for tokenizer initialization - HF_HOME, TRANSFORMERS_CACHE for cached tokenizer access - Add Python debugging environment variables (PYTHONUNBUFFERED, PYTHONFAULTHANDLER) - Add model PVC support for benchmark pods to access cached models ## New Files: ### Documentation - docs/DATASET_STORAGE.md: Guide for dataset storage approaches (Docker image vs PVC) - Includes dataset download instructions from MLPerf repository ### Scripts - scripts/upload-datasets-to-pvc.sh: Upload MLPerf datasets to Kubernetes PVC - build-and-push-harness.sh: Build and push MLPerf harness Docker image ### Examples - examples/study_config_test_gpt_oss.yaml: Full GPT-OSS-120B optimization config (6396 samples) - examples/study_config_test_gpt_oss_QUICK_TEST.yaml: Quick test config (10 samples) - results-pvc-mlperf.yaml: Kubernetes PVC for benchmark results storage ### Docker - Dockerfile.harness-updated: MLPerf harness container with dataset and tokenizer caching ## Key Features: 1. **MLflow Integration**: Full S3 artifact storage support with AWS credentials 2. **Tokenizer Support**: Pre-cached HuggingFace tokenizers for GPT-OSS-120B (200K vocab) 3. **Dataset Management**: Both Docker image and PVC-based dataset storage approaches 4. **Large Context Support**: Configured for GPT-OSS-120B's 131K token context (YaRN scaling) 5. **Timeout Handling**: Dynamic timeout calculation based on benchmark duration ## Tested With: - Model: openai/gpt-oss-120b (120B parameters, MoE, MXFP4 quantization) - vLLM: v0.14.1 - MLPerf: v6.0 (commit 2a90efe) - Scenario: Offline (batch processing) - Expected Performance: ~9K tokens/second on 8xH100 ## Note: Dataset files (>500MB) are not included in this commit due to GitHub size limits. See docs/DATASET_STORAGE.md for download instructions. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces Helm-based and Kubernetes-backed execution backends for vLLM with MLPerf benchmark provider support, extended configuration for container deployments, tunable benchmark parameters, comprehensive Helm chart examples, and CLI extensions for orchestrated trial execution across distributed infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant StudyController as StudyController
participant HelmBackend as HelmExecutionBackend
participant HelmUtils as helm_utils
participant K8sAPI as Kubernetes API
participant VLLMSvc as vLLM Service
participant BenchmarkJob as Benchmark Job
User->>StudyController: submit_trial(trial_config)
activate StudyController
StudyController->>HelmBackend: submit_trial(trial_config)
activate HelmBackend
HelmBackend->>HelmUtils: generate_helm_values(trial_config)
HelmUtils-->>HelmBackend: values_dict
HelmBackend->>K8sAPI: helm upgrade --install release_name
K8sAPI->>K8sAPI: Deploy vLLM via chart
K8sAPI-->>HelmBackend: release_created
HelmBackend->>HelmUtils: wait_for_service_ready(service_url)
HelmUtils->>VLLMSvc: Poll /v1/models until ready
VLLMSvc-->>HelmUtils: 200 OK
HelmUtils-->>HelmBackend: service_ready
HelmBackend->>HelmUtils: create_benchmark_job(service_url, benchmark_config)
HelmUtils->>K8sAPI: kubectl apply benchmark_job_manifest
K8sAPI->>BenchmarkJob: Schedule Job
K8sAPI-->>HelmUtils: job_created
HelmUtils-->>HelmBackend: job_name
HelmBackend->>HelmUtils: wait_for_job_completion(job_name, timeout)
HelmUtils->>BenchmarkJob: Poll job.status
BenchmarkJob-->>HelmUtils: job_completed
HelmUtils-->>HelmBackend: completed
HelmBackend->>HelmUtils: extract_job_results(job_name)
HelmUtils->>BenchmarkJob: Read logs/results
HelmUtils-->>HelmBackend: results_dict
HelmBackend->>K8sAPI: helm uninstall release_name
K8sAPI->>K8sAPI: Clean up vLLM release
K8sAPI-->>HelmBackend: release_deleted
HelmBackend-->>StudyController: TrialResult
deactivate HelmBackend
StudyController-->>User: trial_complete
deactivate StudyController
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@auto_tune_vllm/execution/trial_controller.py`:
- Around line 2325-2331: The WorkloadActor Ray class defines run_benchmark but
RayExecutionBackend.submit_trial expects run_workload, causing AttributeError;
fix by aligning names — either add a run_workload method to the WorkloadActor
that delegates to run_benchmark (or rename run_benchmark to run_workload) so the
actor exposes the method called by RayExecutionBackend.submit_trial; update only
the actor definition (class WorkloadActor) or the caller (method submit_trial)
to use the same method name consistently.
🟠 Major comments (24)
.gitignore-33-33 (1)
33-33: Revert .gitignore to ignore entireoptuna_studies/directory, not justv4/.The code dynamically creates study directories under
optuna_studies/{study_name}/for any study name (e.g.,vllm_quick_test,helm_optimization_example). Narrowing the ignore pattern to onlyoptuna_studies/v4/means studies created with other names will be tracked by git, causing large SQLite databases and study artifacts to be committed unintentionally.The
v4reference in this PR relates to MLPerf dataset versions, not optuna_studies directory structure. Change line 33 back tooptuna_studies/.examples/study_config_guidellm_k8s_direct.yaml-5-9 (1)
5-9: Avoid hardcoded database credentials in the example config.Even in examples, embedding credentials encourages insecure usage and can leak into real configs. Prefer placeholders and note that real values should be sourced from secrets or env injection at runtime.
🔒 Suggested placeholder update
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna-k8s-direct" + database_url: "postgresql://<db_user>:<db_password>@localhost:5432/optuna-k8s-direct" ... - database_url: "postgresql://postgres:postgres@localhost:5432/optuna-k8s-direct" + database_url: "postgresql://<db_user>:<db_password>@localhost:5432/optuna-k8s-direct"Also applies to: 77-80
Dockerfile.harness-updated-13-13 (1)
13-13: Avoid world-writable dataset permissions.
chmod -R 777is overly permissive; prefer group-writable with a known group to keep OpenShift-friendly behavior while reducing risk.🔒 Suggested hardening
-RUN chmod -R 777 /vllm-workspace/mlperf-inference-6.0-redhat/harness/data/v4 +RUN chgrp -R 0 /vllm-workspace/mlperf-inference-6.0-redhat/harness/data/v4 \ + && chmod -R g+rwX /vllm-workspace/mlperf-inference-6.0-redhat/harness/data/v4examples/helm/postgres-test-pod.yaml-7-37 (1)
7-37: Harden the pod securityContext (least privilege).Static analysis flags
allowPrivilegeEscalationand root usage; add explicit securityContext to align with cluster policies.🔒 Suggested securityContext
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: postgres-client image: postgres:15 + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: ["ALL"] command: ["/bin/bash"] args: ["-c", "sleep infinity"]examples/study_config_test_gpt_oss_QUICK_TEST.yaml-9-10 (1)
9-10: Replace hardcoded database credentials with environment variable references.
Credentials should not be committed to the repository, even in test/example configs. The config loader supports${VAR_NAME}and${VAR_NAME:-default}patterns for environment variable expansion.Suggested change
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna-k8s-direct" + database_url: "${DATABASE_URL:-postgresql://user:password@localhost:5432/optuna-k8s-direct}"Apply to both lines 9-10 (study.database_url) and 51-53 (logging.database_url).
scripts/upload-datasets-to-pvc.sh-1-33 (1)
1-33: Fail fast and guarantee uploader pod cleanup.Without error handling, if any kubectl command fails (lines 12-46), the script either continues executing or exits without running cleanup. The uploader pod persists in the cluster indefinitely, wasting resources.
🛡️ Suggested reliability fix
#!/bin/bash # Upload MLPerf datasets to model PVC # This is an alternative to baking datasets into the Docker image +set -euo pipefail + NAMESPACE="autotune-aanya" PVC_NAME="model-cache-pvc" LOCAL_DATASET_DIR="/Users/aansharm/mlperf-build/mlperf-datasets" + +cleanup() { + kubectl delete pod dataset-uploader -n "$NAMESPACE" --ignore-not-found +} +trap cleanup EXITexamples/readiness-check-pod-debug.yaml-9-12 (1)
9-12: Harden the pod's security context (non‑root, no privilege escalation).
This pod runs with default privileges; add a strict securityContext to enforce least-privilege operation.🔐 Suggested hardening
spec: restartPolicy: Never + securityContext: + seccompProfile: + type: RuntimeDefault containers: - name: readiness-check image: quay.io/rh-ee-aansharm/curl:latest # Using quay.io to avoid Docker Hub rate limits + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 65532 + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"]examples/helm/templates/deployment.yaml-22-25 (1)
22-25: Fail fast when container image is unset.
If.imageis missing, Helm renders<no value>and Kubernetes attempts to pull an invalid image. Userequiredto surface the error early.🛠️ Proposed fix
- image: {{ .image }} + image: {{ required "decode.containers[].image is required" .image }}examples/helm/values.yaml-85-89 (1)
85-89: Avoid shipping a known default PostgreSQL password.
postgres/postgresis an insecure default and can leak into real deployments. Prefer leaving it unset and requiring an explicit override or secret.🔒 Proposed fix
auth: username: postgres - password: postgres # Change this in production! + password: "" # REQUIRED: set via --set or a secret; avoid known defaultspyproject.toml-35-43 (1)
35-43: Ray must be in core dependencies, not optional.Ray is explicitly validated as a required package (raising
RuntimeErrorif missing) and is mandatory for all execution modes. Movingray[default]to the optionallocalextra breaks default installations, causing runtime validation failures. Moving Ray to core dependencies and removing it from thelocalextra is required.🛠️ Proposed fix
dependencies = [ "optuna>=3.0.0", "pyyaml>=6.0", "pydantic>=2.0.0", "typer>=0.9.0", "rich>=13.0.0", "requests>=2.28.0", + "ray[default]>=2.0.0", ] [project.optional-dependencies] # Core dependencies for local/Ray execution local = [ "vllm>=0.11.0", "guidellm>=0.1.0", - "ray[default]>=2.0.0", "optuna-integration[botorch]>=4.0.0", "gpytorch>=1.1", ]auto_tune_vllm/benchmarks/providers.py-505-512 (1)
505-512: Avoid piping stdout/stderr without consumption.
stdout/stderr=PIPEcan deadlock if the harness writes enough output. Either stream logs in a background reader or inherit parent streams.🔧 Minimal safe change
self._process = subprocess.Popen( cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + stdout=None, + stderr=None, text=True, env=env, start_new_session=True )auto_tune_vllm/benchmarks/providers.py-642-650 (1)
642-650: Fix Offline scenario batch_size validation.The current comparison checks
num_samplesagainst itself, so mismatches are never detected. This can silently produce invalid runs.🐛 Proposed fix
if config.scenario == "Offline": - if config.num_samples is not None and config.num_samples != config.num_samples: + if ( + config.batch_size is not None + and config.batch_size != config.num_samples + ): raise ValueError( "num_samples must be equal to batch_size for Offline scenario" ) if config.batch_size is None: config.batch_size = config.num_samples cmd.extend(["--batch-size", str(config.batch_size)])auto_tune_vllm/benchmarks/providers.py-556-563 (1)
556-563: Ruff E501: wrap long error/log lines.These lines exceed the configured limit and will keep CI red.
✂️ Suggested wrap
if "output_tokens_per_second" not in data: raise RuntimeError( - f"Required metric 'output_tokens_per_second' not found in MLPerf results. " - f"Available keys: {list(data.keys())}" + "Required metric 'output_tokens_per_second' not found in MLPerf results. " + f"Available keys: {list(data.keys())}" ) - self._logger.info( - f"Parsed MLPerf results: output_tokens_per_second={data['output_tokens_per_second']}" - ) + self._logger.info( + "Parsed MLPerf results: output_tokens_per_second=%s", + data["output_tokens_per_second"], + )auto_tune_vllm/benchmarks/providers.py-461-463 (1)
461-463: Parameterize subprocess.Popen with [str] to match the text=True argument.The
start_benchmarkmethod instantiatessubprocess.Popenwithtext=True(line 509), which means stdout and stderr operate in text mode. The return type should besubprocess.Popen[str]to properly reflect this in the type system.🔤 Typing fix
- def start_benchmark( - self, model_url: str, config: BenchmarkConfig - ) -> subprocess.Popen: + def start_benchmark( + self, model_url: str, config: BenchmarkConfig + ) -> subprocess.Popen[str]:examples/study_config_mlperf_offline_k8s.yaml-7-12 (1)
7-12: Remove plaintext DB credentials from example config.Both database URLs embed credentials; this will trigger secret scanners and encourages committing secrets. Use placeholders instead.
🔒 Suggested change
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna" + database_url: "postgresql://<user>:<password>@<host>:<port>/<db>" ... - database_url: "postgresql://postgres:postgres@localhost:5432/tuner-user" + database_url: "postgresql://<user>:<password>@<host>:<port>/<db>"Also applies to: 70-73
examples/study_config_test_gpt_oss.yaml-6-8 (1)
6-8: Remove credentials and user-specific paths from example config.These values will trigger secret scanning and leak personal info. Prefer placeholders.
🔒 Suggested change
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna-k8s-direct" + database_url: "postgresql://<user>:<password>@<host>:<port>/<db>" ... - kubeconfig: "/Users/aansharm/.kube/config" # Optional: custom kubeconfig path + kubeconfig: "/path/to/your/kubeconfig" # Optional: custom kubeconfig path ... - database_url: "postgresql://postgres:postgres@localhost:5432/optuna-k8s-direct" + database_url: "postgresql://<user>:<password>@<host>:<port>/<db>"Also applies to: 20-21, 59-61
auto_tune_vllm/execution/k8s_utils.py-104-112 (1)
104-112: Fix model-argument detection to avoid false positives.
any("--model" in arg for arg in args)will match flags like--max-model-len, causing the actual--modelto be skipped and vLLM to fail.🐛 Proposed fix
- model_in_args = any("--model" in arg for arg in args) + model_in_args = any( + arg == "--model" or arg.startswith("--model=") + for arg in args + )scripts/cleanup_study_resources.sh-147-153 (1)
147-153: Auto-detected namespace always picks the first project.
head -1means the “only one project” check always passes, so a multi-project cluster can silently select the wrong namespace—dangerous for a cleanup script.🛠️ Safer detection
- projects=$($KUBECTL_CMD get projects -o name 2>/dev/null | head -1 | sed 's|project.project.openshift.io/||' || echo "") - if [[ -n "$projects" ]] && [[ $(echo "$projects" | wc -l) -eq 1 ]]; then - NAMESPACE="$projects" + projects=$($KUBECTL_CMD get projects -o name 2>/dev/null | sed 's|project.project.openshift.io/||' || echo "") + project_count=$(echo "$projects" | sed '/^$/d' | wc -l | tr -d ' ') + if [[ "$project_count" -eq 1 ]]; then + NAMESPACE="$(echo "$projects" | head -1)" verbose "Using only available project: $NAMESPACE" + elif [[ "$project_count" -gt 1 ]]; then + error "Multiple projects detected; please specify --namespace" + exit 1 fiscripts/cleanup_study_resources.sh-329-350 (1)
329-350: Filter readiness-check job cleanup by study to prevent interfering with concurrent studies.The
cleanup_readiness_jobs()function accepts asanitized_studyparameter but does not use it. The function currently deletes all readiness-check jobs in the namespace with labelapp=auto-tune-vllm-readiness-check, regardless of which study they belong to. Since job names are UUID-based and jobs lack study-specific labels, this will delete readiness-check jobs from other studies running in the same namespace.Add a study-specific label when creating readiness-check jobs and filter by it during cleanup, or use the job name pattern to match the study context.
examples/helm/test-postgres.sh-114-116 (1)
114-116: Add namespace flag to kubectl apply for consistency.The manifest
postgres-test-pod.yamlhasmetadata.namespace: llm-d-trialshardcoded. Thekubectl applyat line 116 lacks the-nflag, which works currently only becauseNAMESPACEis hardcoded to match. To ensure consistency with subsequentkubectl waitandkubectl execcommands that explicitly target$NAMESPACE, add the namespace flag:Proposed fix
-kubectl apply -f "$TEST_POD_FILE" +kubectl apply -n "$NAMESPACE" -f "$TEST_POD_FILE"auto_tune_vllm/cli/main.py-231-285 (1)
231-285: Avoid deleting the entire SQLite DB and guardconninitialization.
If the study name isn’t found, the code deletes the whole DB file, which can wipe unrelated studies. Also,connis referenced infinallywithout being initialized ifsqlite3.connect()fails.🔧 Safer deletion flow
if storage_path.exists(): - # Connect to SQLite and check/delete study - conn = sqlite3.connect(str(storage_path)) + # Connect to SQLite and check/delete study + conn = None + conn = sqlite3.connect(str(storage_path)) try: cursor = conn.cursor() ... if study_row: ... else: - # Study not found in database, but file exists - delete the file to clear any cached state - conn.close() - storage_path.unlink() - console.print(f"[green]✅ Deleted SQLite database file to clear cached study state[/green]") + console.print( + f"[yellow]Study '{study_config.study_name}' not found in SQLite storage; skipping delete[/yellow]" + ) finally: - if conn: + if conn is not None: conn.close()auto_tune_vllm/execution/backends.py-806-990 (1)
806-990: Pass--kubeconfigto all Helm CLI calls when provided.
self.kubeconfigis stored in__init__but none of the helm commands in the deploy method pass it—creating an inconsistency with the cleanup methods which correctly include--kubeconfig. This causes deploy operations to use the default kubeconfig (~/.kube/config) instead of the provided one, risking operations against the wrong cluster.🔧 Suggested helper
+ def _helm(cmd: list[str]) -> list[str]: + if self.kubeconfig: + return cmd + ["--kubeconfig", self.kubeconfig] + return cmd ... - result = subprocess.run( - ["helm", "list", "-n", self.namespace, "-q"], + result = subprocess.run( + _helm(["helm", "list", "-n", self.namespace, "-q"]), capture_output=True, text=True, ) ... - result = subprocess.run(infra_cmd, check=True, capture_output=True, text=True) + result = subprocess.run(_helm(infra_cmd), check=True, capture_output=True, text=True) ... - result = subprocess.run(gaie_cmd, check=True, capture_output=True, text=True) + result = subprocess.run(_helm(gaie_cmd), check=True, capture_output=True, text=True) ... - result = subprocess.run( - helm_cmd, + result = subprocess.run( + _helm(helm_cmd), check=True, capture_output=True, text=True, )auto_tune_vllm/execution/trial_controller.py-1489-1701 (1)
1489-1701: Add kubeconfig parameter to HelmTrialController and propagate through benchmark operations.HelmExecutionBackend accepts a
kubeconfigparameter but cannot pass it to HelmTrialController because the controller's__init__doesn't accept it. This causes_start_benchmarkto hardcodekubeconfig=None, ignoring the specified cluster configuration. The backend calls site inbackends.pymust also be updated to pass the kubeconfig.🔧 Suggested changes
Update
HelmTrialController.__init__to accept and store kubeconfig:- def __init__(self, release_name: str, namespace: str = "default", benchmark_image: Optional[str] = None, helm_config: Optional[Dict[str, Any]] = None, benchmark_pvc: Optional[str] = None, model_pvc: Optional[str] = None): + def __init__( + self, + release_name: str, + namespace: str = "default", + benchmark_image: Optional[str] = None, + helm_config: Optional[Dict[str, Any]] = None, + benchmark_pvc: Optional[str] = None, + model_pvc: Optional[str] = None, + kubeconfig: Optional[str] = None, + ):Store kubeconfig in
__init__:self.benchmark_pvc = benchmark_pvc self.model_pvc = model_pvc + self.kubeconfig = kubeconfigUpdate
_start_benchmarkto pass it:- self.benchmark_job_name = create_benchmark_job( - trial_config, self.server_url, self.namespace, benchmark_image, kubeconfig=None, benchmark_pvc=self.benchmark_pvc, model_pvc=self.model_pvc - ) + self.benchmark_job_name = create_benchmark_job( + trial_config, + self.server_url, + self.namespace, + benchmark_image, + kubeconfig=self.kubeconfig, + benchmark_pvc=self.benchmark_pvc, + model_pvc=self.model_pvc, + )Update the instantiation in
backends.py:- controller = HelmTrialController(release_name, self.namespace, benchmark_image, self.helm_config, benchmark_pvc=benchmark_pvc, model_pvc=model_pvc) + controller = HelmTrialController( + release_name, + self.namespace, + benchmark_image, + self.helm_config, + benchmark_pvc=benchmark_pvc, + model_pvc=model_pvc, + kubeconfig=self.kubeconfig, + )auto_tune_vllm/cli/main.py-305-329 (1)
305-329: Honor--kubeconfigwhen listing/uninstalling Helm releases.When a kubeconfig is provided, the cleanup currently uses the default Kubernetes context. This can clean up releases from the wrong cluster.
🔧 Suggested change
- result = subprocess.run( - ["helm", "list", "-n", namespace, "-q"], + helm_list_cmd = ["helm", "list", "-n", namespace, "-q"] + if kubeconfig: + helm_list_cmd.extend(["--kubeconfig", kubeconfig]) + result = subprocess.run( + helm_list_cmd, capture_output=True, text=True, ) ... - subprocess.run( - ["helm", "uninstall", release, "-n", namespace], + uninstall_cmd = ["helm", "uninstall", release, "-n", namespace] + if kubeconfig: + uninstall_cmd.extend(["--kubeconfig", kubeconfig]) + subprocess.run( + uninstall_cmd, capture_output=True, text=True, )
🟡 Minor comments (14)
docs/ray_cluster_setup.md-372-395 (1)
372-395: Fix namespaceSelector label key in NetworkPolicy example.Most clusters label namespaces with
kubernetes.io/metadata.name, notname, so this policy won’t match and can block traffic.✅ Suggested fix
ingress: - from: - namespaceSelector: matchLabels: - name: <your-namespace> + kubernetes.io/metadata.name: <your-namespace> egress: - to: - namespaceSelector: matchLabels: - name: <your-namespace> + kubernetes.io/metadata.name: <your-namespace>docs/DATASET_STORAGE.md-86-98 (1)
86-98: Replace user‑specific paths/namespaces with placeholders.
These examples hardcode a personal namespace and local path, which will mislead most users.✍️ Suggested doc edits
-kubectl run dataset-uploader --image=busybox -n autotune-aanya \ +kubectl run dataset-uploader --image=busybox -n <your-namespace> \ @@ -kubectl cp mlperf-build/mlperf-datasets/v4 \ - autotune-aanya/dataset-uploader:/mnt/models/datasets/v4 +kubectl cp /path/to/mlperf-build/mlperf-datasets/v4 \ + <your-namespace>/dataset-uploader:/mnt/models/datasets/v4 @@ -cd /Users/aansharm/mlperf-build +cd /path/to/mlperf-buildAlso applies to: 125-127
examples/helm/README.md-879-894 (1)
879-894: Add language tags to fenced blocks to satisfy MD040.
markdownlint flags these two fences because they lack a language identifier. Addingtextkeeps the docs lint-clean.🛠️ Proposed fix
- ``` + ```text --model Qwen/Qwen3-0.6B --port 8000 --tensor-parallel-size 2 --data-parallel-size 1 --served-model-name Qwen/Qwen3-0.6B ``` @@ - ``` + ```text --max-num-seqs 128 --gpu-memory-utilization 0.9 --max-model-len 16384 ```examples/study_config.yaml-37-37 (1)
37-37: Database name mismatch between study and logging sections.The study section uses
optunawhile the logging section usestuner-user. This inconsistency appears systematically across multiple config files. However, Kubernetes and Helm deployment variants (e.g.,study_config_mlperf_helm.yaml) consistently use matching database names in both sections, suggesting this may be unintentional. Either clarify if separate databases are required or align the logging database name with the study database to match the pattern used in the k8s/helm configs.auto_tune_vllm/execution/__init__.py-3-8 (1)
3-8: AddHelmExecutionBackendto exports for API consistency.
HelmExecutionBackendis defined inbackends.pyand is used throughout the codebase (e.g., instudy_controller.pyandcli/main.py), but it is not imported or exported from this module. All other execution backend implementations are exported, soHelmExecutionBackendshould be added to the import statement (lines 3-8) and the__all__list (lines 11-17) for consistency.examples/study_config_mlperf_server_k8s.yaml-7-9 (1)
7-9: Inconsistent database names between study and logging sections.The study section uses database
optuna(line 9) while the logging section usestuner-user(line 79). This may be intentional for separation of concerns, but could confuse users who expect a single database. Consider using the same database or adding a comment explaining why they differ.Suggested fix if using same database
logging: # At least one of the following must be provided and must be valid - database_url: "postgresql://postgres:postgres@localhost:5432/tuner-user" + database_url: "postgresql://postgres:postgres@localhost:5432/optuna" file_path: "/tmp/auto-tune-vllm-logs" # Local file path (on the machine running auto-tune) log_level: "INFO"Also applies to: 77-79
examples/study_config_mlperf_server_k8s_direct.yaml-88-89 (1)
88-89: Comment contradicts the actual value.The comment states "Use 1 GPU per trial" but
tensor_parallel_sizeis set to 8.Suggested fix
static_parameters: - tensor_parallel_size: 8 # Use 1 GPU per trial + tensor_parallel_size: 8 # Use 8 GPUs per trial (tensor parallelism)examples/study_config_mlperf_server.yaml-5-8 (1)
5-8: Avoid hardcoded database credentials in example config.Using default creds in examples can lead to unsafe copy‑paste usage. Placeholders are safer.
🔐 Proposed edit (placeholders)
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna" + database_url: "postgresql://<user>:<password>@localhost:5432/optuna" ... - database_url: "postgresql://postgres:postgres@localhost:5432/tuner-user" + database_url: "postgresql://<user>:<password>@localhost:5432/tuner-user"Also applies to: 44-48
examples/study_config_mlperf_offline.yaml-5-8 (1)
5-8: Avoid hardcoded database credentials in example configs.Even in examples, baked‑in credentials tend to get copied into real deployments. Consider using placeholders to avoid accidental reuse.
🔐 Proposed edit (placeholders)
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna" + database_url: "postgresql://<user>:<password>@localhost:5432/optuna" ... - database_url: "postgresql://postgres:postgres@localhost:5432/tuner-user" + database_url: "postgresql://<user>:<password>@localhost:5432/tuner-user"Also applies to: 37-40
examples/study_config_mlperf_helm.yaml-4-12 (1)
4-12: Avoid hardcoded database credentials in example config.These values are likely to be copy‑pasted into real deployments. Consider placeholders to prevent accidental reuse.
🔐 Proposed edit (placeholders)
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna" + database_url: "postgresql://<user>:<password>@localhost:5432/optuna" ... - database_url: "postgresql://postgres:postgres@localhost:5432/optuna" + database_url: "postgresql://<user>:<password>@localhost:5432/optuna"Also applies to: 70-74
auto_tune_vllm/execution/trial_controller.py-2094-2101 (1)
2094-2101: Same validation gap in KubernetesTrialController.run_trial().
Apply the same_validate_environment()call here for parity and early failure.auto_tune_vllm/execution/trial_controller.py-1726-1734 (1)
1726-1734: Run environment validation in Helm/K8s controllers.
BaseTrialController.run_trial()validates dependencies, but Helm/K8s overriderun_trial()and skip_validate_environment(), so missing dependencies only surface later.🔧 Suggested addition
try: # Setup trial-specific logging self._setup_trial_logging(trial_config) + # Validate controller-side dependencies + self._validate_environment(trial_config)auto_tune_vllm/cli/main.py-155-191 (1)
155-191: Ruff E501: wrap long option/help lines.
CI flags these lines as too long (E501). Consider wrapping the help strings and multi-part prints.🧹 Example wrapping
- helm_chart_name: Optional[str] = typer.Option( - None, "--helm-chart-name", help="Helm chart name from repository (for Helm backend)" - ), + helm_chart_name: Optional[str] = typer.Option( + None, + "--helm-chart-name", + help="Helm chart name from repository (for Helm backend)", + ), ... - console.print( - "[bold red]" - "Error: At least one Python environment option must be specified for Ray backend" - "[/bold red]" - ) + console.print( + "[bold red]" + "Error: At least one Python environment option must be specified " + "for Ray backend" + "[/bold red]" + )auto_tune_vllm/core/config.py-700-729 (1)
700-729: Extend dataset-vs-synthetic validation to all synthetic-only fields.
Onlyprompt_tokens/output_tokensare blocked whendatasetis set; the stdev/min/max synthetic fields can still slip through and create contradictory configs.🔧 Suggested validation
- # Validate dataset vs synthetic data parameters + # Validate dataset vs synthetic data parameters # If dataset is provided, prompt_tokens and output_tokens cannot be specified dataset_value = benchmark_constants.get("dataset") if dataset_value is not None: - # Check constants - these are synthetic data parameters - synthetic_params_in_constants = [] - if "prompt_tokens" in benchmark_constants: - synthetic_params_in_constants.append("prompt_tokens") - if "output_tokens" in benchmark_constants: - synthetic_params_in_constants.append("output_tokens") + synthetic_fields = { + "prompt_tokens", + "output_tokens", + "prompt_tokens_stdev", + "prompt_tokens_min", + "prompt_tokens_max", + "output_tokens_stdev", + "output_tokens_min", + "output_tokens_max", + } + synthetic_params_in_constants = [ + k for k in benchmark_constants.keys() if k in synthetic_fields + ] ... - synthetic_params_in_tunables = [] - if "prompt_tokens" in benchmark_tunables_raw: - synthetic_params_in_tunables.append("prompt_tokens") - if "output_tokens" in benchmark_tunables_raw: - synthetic_params_in_tunables.append("output_tokens") + synthetic_params_in_tunables = [ + k for k in benchmark_tunables_raw.keys() if k in synthetic_fields + ]
🧹 Nitpick comments (23)
docs/ray_cluster_setup.md (1)
315-403: Add explicit vLLM bind-address guidance for multi-node Ray.Please add a short note that vLLM servers should bind to
0.0.0.0in distributed/KubeRay setups so other worker nodes can reach them (otherwise “localhost” binding breaks cross-node access). Based on learnings, ...examples/study_config_guidellm_k8s_direct.yaml (1)
16-17: Pin the vLLM image tag instead oflatestfor reproducible benchmarking.The
latesttag is non-deterministic and can change behavior unexpectedly across runs. Pin to a specific stable release (e.g.,v0.14.0, the current stable as of January 2026) for consistent results.♻️ Example pinning
- vllm_image: "vllm/vllm-openai:latest" # vLLM container image + vllm_image: "vllm/vllm-openai:v0.14.0" # vLLM container imageAlternatively, use a platform/CUDA-specific variant like
v0.14.0-x86_64-cu130for tighter control.results-pvc-mlperf.yaml (1)
3-9: Make namespace and StorageClass explicitly configurable.Hardcoding these values can break in other clusters; add guidance or placeholders to improve portability.
✏️ Suggested clarification
metadata: name: results-pvc-mlperf - namespace: autotune-aanya + # TODO: replace with your target namespace + namespace: autotune-aanya spec: accessModes: - ReadWriteOnce - storageClassName: lvms-lvm-vg-nvme-dgx + # TODO: replace with your cluster's StorageClass + storageClassName: lvms-lvm-vg-nvme-dgxREADME.md (1)
31-38: Add Helm/Kubectl prerequisites for the Helm-based path.Readers may miss required CLI tooling and kubeconfig context.
📝 Suggested doc note
#### For Helm-based Kubernetes Execution (Remote vLLM) ```bash git clone https://github.com/openshift-psap/auto-tuning-vllm.git cd auto-tuning-vllm pip install -e .[helm]+Prereqs: Helm v3+, kubectl, and a kubeconfig pointing to the target cluster.
+
Note: For Helm deployments, vLLM and benchmarks run remotely in Kubernetes, so they don't need to be installed on the controller.</details> </blockquote></details> <details> <summary>examples/study_config_optimization_examples.yaml (1)</summary><blockquote> `167-181`: **Clarify the difference between benchmark vs framework log levels.** A short comment helps avoid misconfiguration. <details> <summary>📝 Suggested clarification</summary> ```diff benchmark: # Constants benchmark_type: "guidellm" model: "facebook/opt-125m" max_seconds: 180 dataset: null - logging_level: "INFO" + # Benchmark runner verbosity + logging_level: "INFO" # Tunables (empty dict if no tunables needed) tunables: {} logging: file_path: "/tmp/auto-tune-vllm-logs" + # Auto-tune-vllm framework log level log_level: "INFO"Dockerfile.harness-updated (1)
6-6: Pin MLflow and Blinker versions for reproducible builds.Unpinned installs can break builds over time. Consider pinning to known-good versions (via constraints or a requirements file) to keep images deterministic.
build-and-push-harness.sh (1)
10-21: Make the image/tag configurable and build relative to the script path.
Hardcoded values and relative Dockerfile paths make the script brittle when run from another directory or for other registries.♻️ Suggested refactor
+# Resolve script directory for portable paths +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + # Configuration -IMAGE_NAME="quay.io/rh-ee-nmiriyal/mlperf-6.0" -IMAGE_TAG="harness-updated" -FULL_IMAGE="${IMAGE_NAME}:${IMAGE_TAG}" +IMAGE_NAME="${IMAGE_NAME:-quay.io/rh-ee-nmiriyal/mlperf-6.0}" +IMAGE_TAG="${IMAGE_TAG:-harness-updated}" +FULL_IMAGE="${IMAGE_NAME}:${IMAGE_TAG}" @@ -# Build the image echo "Step 1: Building Docker image..." -docker build -f Dockerfile.harness-updated -t ${FULL_IMAGE} . +docker build -f "${SCRIPT_DIR}/Dockerfile.harness-updated" -t "${FULL_IMAGE}" "${SCRIPT_DIR}"scripts/upload-datasets-to-pvc.sh (1)
5-8: Make namespace/PVC/path configurable and validate the dataset path.
Hardcoded values and a user‑specific local path will break for most users.♻️ Suggested refactor
-NAMESPACE="autotune-aanya" -PVC_NAME="model-cache-pvc" -LOCAL_DATASET_DIR="/Users/aansharm/mlperf-build/mlperf-datasets" +NAMESPACE="${NAMESPACE:-autotune-aanya}" +PVC_NAME="${PVC_NAME:-model-cache-pvc}" +LOCAL_DATASET_DIR="${LOCAL_DATASET_DIR:-/path/to/mlperf-build/mlperf-datasets}" + +if [ ! -d "$LOCAL_DATASET_DIR/v4" ]; then + echo "Error: dataset directory not found: $LOCAL_DATASET_DIR/v4" + exit 1 +fiAlso applies to: 39-42
examples/study_config_test_gpt_oss_QUICK_TEST.yaml (1)
14-20: Replace local paths/IPs with placeholders for portability.
The kubeconfig path and MLflow host are environment‑specific; placeholders avoid confusion for new users.✍️ Suggested doc-style placeholders
- kubeconfig: "/Users/aansharm/.kube/config" + kubeconfig: "/path/to/kubeconfig" @@ - mlflow_host: "150.239.115.202" + mlflow_host: "<mlflow-host-or-ip>"Also applies to: 46-47
examples/helm/templates/deployment.yaml (1)
28-46: Make the--portvalue configurable to avoid service/command mismatches.
Port is hardcoded to 8000 forvllmServe/imageDefault. Ifservice.targetPort(or container ports) differ, the service can route to the wrong port.♻️ Proposed refactor
args: - --port - - "8000" + - {{ $.Values.service.targetPort | quote }} - --host - "0.0.0.0" @@ args: - --port - - "8000" + - {{ $.Values.service.targetPort | quote }} - --host - "0.0.0.0"auto_tune_vllm/benchmarks/config.py (2)
4-4: Consider updating type hints to modern Python 3.10+ syntax.Pyright reports that
OptionalandDictare deprecated as of Python 3.9/3.10. While functional, consider updating to modern syntax for consistency.♻️ Proposed modernization
-from typing import Any, Dict, Literal, Optional +from typing import Any, LiteralThen update usages throughout the file:
Optional[str]→str | NoneOptional[int]→int | NoneDict[str, Any]→dict[str, Any]
25-26: Fix line length violations flagged by Ruff.Lines 25, 49, and 50 exceed the 88-character limit. Move comments to separate lines.
♻️ Proposed fix for line length
- rate: int = 50 # Single rate value for concurrent requests (can be overridden by tunables) - samples: int = 1000 # Number of samples to take (can be overridden by tunables) + # Single rate value for concurrent requests (can be overridden by tunables) + rate: int = 50 + # Number of samples to take (can be overridden by tunables) + samples: int = 1000- server_target_qps: Optional[float] = None # Target QPS for Server scenario (tunable) - server_coalesce_queries: Optional[bool] = None # Coalesce queries flag for Server scenario (tunable) + # Target QPS for Server scenario (tunable) + server_target_qps: Optional[float] = None + # Coalesce queries flag for Server scenario (tunable) + server_coalesce_queries: Optional[bool] = Noneexamples/study_config.yaml (1)
4-4: Consider using environment variable placeholders for credentials in examples.Example configuration files often get copied directly into production. Using hardcoded credentials like
postgres:postgresmay inadvertently encourage insecure practices. The documentation already shows environment variable expansion (${POSTGRES_PASSWORD}).♻️ Proposed fix
- database_url: "postgresql://postgres:postgres@localhost:5432/optuna" + database_url: "postgresql://postgres:${POSTGRES_PASSWORD:-postgres}@localhost:5432/optuna"This demonstrates the environment variable pattern while still working out-of-the-box with a default value for quick local testing.
examples/trial_config_comprehensive.yaml (1)
18-18: Clarify relationship between logging configurations.There are two logging-related configurations:
logging_levelwithin thebenchmarkblock (line 18) andlog_levelwithin the dedicatedloggingblock (line 45). If these serve different purposes (e.g., benchmark-specific vs. global application logging), consider adding a brief comment to clarify their scopes.Also applies to: 43-45
examples/helm/deploy-postgres-standalone.sh (1)
68-68: Add trailing newline.The file is missing a trailing newline, which is a POSIX convention and can cause issues with some tools.
Proposed fix
echo "Service endpoint:" echo " postgresql.${NAMESPACE}.svc.cluster.local:5432" +examples/helm/templates/postgres-secret.yaml (1)
12-13: Add validation for required credentials.If
.Values.postgresql.auth.usernameor.Values.postgresql.auth.passwordis not set, the Secret will contain empty credentials, which could cause PostgreSQL authentication failures. Consider using therequiredfunction to fail fast during template rendering.Proposed fix
data: - postgres-user: {{ .Values.postgresql.auth.username | b64enc | quote }} - postgres-password: {{ .Values.postgresql.auth.password | b64enc | quote }} + postgres-user: {{ required "postgresql.auth.username is required" .Values.postgresql.auth.username | b64enc | quote }} + postgres-password: {{ required "postgresql.auth.password is required" .Values.postgresql.auth.password | b64enc | quote }}examples/study_config_mlperf_server_k8s_direct.yaml (1)
20-20: User-specific path should be a placeholder in example configs.The kubeconfig path
/Users/aansharm/.kube/configis user-specific and won't work for others. Use a placeholder like$HOME/.kube/configor"/path/to/your/kubeconfig"to make it clear this needs customization.Suggested fix
- kubeconfig: "/Users/aansharm/.kube/config" # Optional: custom kubeconfig path + kubeconfig: "$HOME/.kube/config" # Optional: custom kubeconfig pathdocs/helm_deployment.md (1)
179-182: Consider noting the security implications of piping curl to bash.While this is the official Helm installation method, security-conscious users may prefer verifying the script first or using package managers. Consider adding an alternative:
# Or use package manager (safer alternative) # macOS: brew install helm # Linux: snap install helm --classicexamples/helm/postgres-standalone.yaml (2)
72-105: Add securityContext to follow Kubernetes security best practices.Even for example files, including a security context sets a good precedent and avoids privilege escalation risks when users copy this configuration.
Suggested addition after line 71
spec: + securityContext: + runAsNonRoot: true + runAsUser: 999 # postgres user + fsGroup: 999 containers: - name: postgresql image: postgres:15 imagePullPolicy: IfNotPresent + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: false + capabilities: + drop: + - ALL
126-135: Storage classnfs-storagemay not exist in all clusters.Consider adding a comment noting this should be changed to match the target cluster's available storage classes, or use a more common default.
Suggested improvement
volumeClaimTemplates: - metadata: name: postgresql-data spec: accessModes: - ReadWriteOnce - storageClassName: nfs-storage + storageClassName: nfs-storage # CHANGE THIS: Use your cluster's available storage class (e.g., standard, gp2, managed-premium) resources: requests: storage: 20Giexamples/study_config_mlperf_server_k8s.yaml (1)
1-6: File name vs content mismatch.The filename is
study_config_mlperf_server_k8s.yamlsuggesting direct Kubernetes, but the content configuresbackend: "helm". Consider renaming tostudy_config_mlperf_server_helm.yamlfor consistency with other example files, or update the header comments to clarify this is for Helm deployment.examples/helm/templates/postgres-statefulset.yaml (1)
23-74: Consider exposing container securityContext via values.This helps deploy cleanly on restricted/PSA clusters and documents least‑privilege defaults.
🔧 Optional values-driven securityContext
containers: - name: postgresql image: {{ .Values.postgresql.image.repository }}:{{ .Values.postgresql.image.tag }} imagePullPolicy: {{ .Values.postgresql.image.pullPolicy | default "IfNotPresent" }} + {{- if .Values.postgresql.securityContext }} + securityContext: + {{- toYaml .Values.postgresql.securityContext | nindent 10 }} + {{- end }} ports:examples/helm/postgres-test-pod-helm.yaml (1)
15-41: Add a minimal securityContext to align with K8s security checks.Even test pods often run under restricted policies; adding a minimal securityContext avoids failures and improves posture.
🔒 Suggested securityContext
containers: - name: postgres-client image: postgres:15 + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + capabilities: + drop: ["ALL"]
| if RAY_AVAILABLE: | ||
| @ray.remote | ||
| class WorkloadActor: | ||
| """Ray actor that runs benchmark workload.""" | ||
| def __init__(self): pass | ||
| def run_benchmark(self, *args, **kwargs): return {} | ||
| else: |
There was a problem hiding this comment.
WorkloadActor method name mismatch will crash Ray trials.
RayExecutionBackend.submit_trial() calls workload_actor.run_workload.remote(...), but the actor defines run_benchmark. This will raise AttributeError at runtime.
🔧 Align method name
if RAY_AVAILABLE:
`@ray.remote`
class WorkloadActor:
"""Ray actor that runs benchmark workload."""
def __init__(self): pass
- def run_benchmark(self, *args, **kwargs): return {}
+ def run_workload(self, *args, **kwargs): return {}
else:
class WorkloadActor:
"""Dummy WorkloadActor when Ray is not available."""
pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if RAY_AVAILABLE: | |
| @ray.remote | |
| class WorkloadActor: | |
| """Ray actor that runs benchmark workload.""" | |
| def __init__(self): pass | |
| def run_benchmark(self, *args, **kwargs): return {} | |
| else: | |
| if RAY_AVAILABLE: | |
| `@ray.remote` | |
| class WorkloadActor: | |
| """Ray actor that runs benchmark workload.""" | |
| def __init__(self): pass | |
| def run_workload(self, *args, **kwargs): return {} | |
| else: |
🤖 Prompt for AI Agents
In `@auto_tune_vllm/execution/trial_controller.py` around lines 2325 - 2331, The
WorkloadActor Ray class defines run_benchmark but
RayExecutionBackend.submit_trial expects run_workload, causing AttributeError;
fix by aligning names — either add a run_workload method to the WorkloadActor
that delegates to run_benchmark (or rename run_benchmark to run_workload) so the
actor exposes the method called by RayExecutionBackend.submit_trial; update only
the actor definition (class WorkloadActor) or the caller (method submit_trial)
to use the same method name consistently.
- Fix server_coalesce_queries argument passing to accept boolean value - Add trial-specific output directories to prevent MLflow artifact accumulation - Update benchmark image to version with api_result parameter fix Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@auto_tune_vllm/benchmarks/providers.py`:
- Around line 643-650: The current check mistakenly compares config.num_samples
to itself; change the condition to compare config.batch_size !=
config.num_samples when config.num_samples is not None to enforce equality for
the "Offline" scenario, and do not mutate the passed-in BenchmarkConfig: compute
a local variable (e.g., batch_size = config.batch_size if config.batch_size is
not None else config.num_samples) and use that local when calling
cmd.extend(["--batch-size", str(batch_size)]) and when validating equality
between config.num_samples and config.batch_size.
In `@examples/study_config_test_gpt_oss.yaml`:
- Line 20: Replace the user-specific hardcoded kubeconfig value by changing the
"kubeconfig" YAML field to a generic placeholder or commented instruction (e.g.,
set kubeconfig: "<PATH_TO_YOUR_KUBECONFIG>" or comment that users should replace
with their own path) so the examples/study_config_test_gpt_oss.yaml is portable
and not tied to a specific user's home directory.
- Around line 1-3: Update the top header comment to match the actual configured
scenario: replace the phrase "Server Scenario" in the header comment with
"Offline Scenario" (the configuration uses scenario: "Offline"); ensure the
header consistently describes the file's purpose and references the scenario key
scenario: "Offline" so the comment and the config stay in sync.
🧹 Nitpick comments (5)
auto_tune_vllm/benchmarks/providers.py (3)
556-564: Fix line length violations.Lines 558 and 563 exceed the 88-character limit per ruff E501.
✨ Proposed fix
# Validate required metric for optimization if "output_tokens_per_second" not in data: raise RuntimeError( - f"Required metric 'output_tokens_per_second' not found in MLPerf results. " - f"Available keys: {list(data.keys())}" + "Required metric 'output_tokens_per_second' not found in " + f"MLPerf results. Available keys: {list(data.keys())}" ) - self._logger.info( - f"Parsed MLPerf results: output_tokens_per_second={data['output_tokens_per_second']}" - ) + tokens_per_sec = data['output_tokens_per_second'] + self._logger.info(f"Parsed MLPerf results: output_tokens_per_second={tokens_per_sec}") return data
567-604: Extract duplicate_get_results_file_pathto base class.This method is identical to
GuideLLMBenchmark._get_results_file_path(lines 276-313). Moving it to theBenchmarkProviderbase class would eliminate duplication and ensure consistent behavior across providers.
656-660: Fix line length violations.Lines 644 and 658 exceed the 88-character limit per ruff E501.
✨ Proposed fix for line 658
if config.server_coalesce_queries is not None: - cmd.extend(["--server-coalesce-queries", str(config.server_coalesce_queries).lower()]) + coalesce = str(config.server_coalesce_queries).lower() + cmd.extend(["--server-coalesce-queries", coalesce])examples/study_config_test_gpt_oss.yaml (2)
5-9: Consider using environment variable references for credentials.Plain-text credentials in configuration files (even examples) can be accidentally committed to production. Consider documenting that these are placeholders or using environment variable substitution syntax.
✨ Suggested improvement
study: prefix: "autotune_test_gpt_oss" - database_url: "postgresql://postgres:postgres@localhost:5432/optuna-k8s-direct" + database_url: "${OPTUNA_DATABASE_URL}" # e.g., postgresql://user:pass@host:5432/db # For Kubernetes: Use port-forwarding or cluster-internal database URL # Example with port-forward: kubectl port-forward -n <namespace> svc/postgresql 5432:5432
51-52: Replace hardcoded IP address with placeholder or hostname.The hardcoded IP
150.239.115.202formlflow_hostmay become stale. Consider using a hostname, environment variable, or clearly documenting this as a placeholder.✨ Suggested improvement
mlflow_experiment_name: "autotune-gpt-oss-120b-offline" # MLflow experiment name - mlflow_host: "150.239.115.202" # MLflow host address (must be accessible from pods) + mlflow_host: "${MLFLOW_HOST}" # MLflow host address (must be accessible from pods)
| if config.scenario == "Offline": | ||
| if config.num_samples is not None and config.num_samples != config.num_samples: | ||
| raise ValueError( | ||
| "num_samples must be equal to batch_size for Offline scenario" | ||
| ) | ||
| if config.batch_size is None: | ||
| config.batch_size = config.num_samples | ||
| cmd.extend(["--batch-size", str(config.batch_size)]) |
There was a problem hiding this comment.
Critical logic error: condition always evaluates to False.
Line 644 compares config.num_samples to itself (config.num_samples != config.num_samples), which is always False. This should likely compare config.batch_size to config.num_samples.
Additionally, line 649 directly mutates the passed-in config object, which can cause unexpected side effects since BenchmarkConfig may be reused.
🐛 Proposed fix
# Scenario-specific parameters
if config.scenario == "Offline":
- if config.num_samples is not None and config.num_samples != config.num_samples:
+ if config.batch_size is not None and config.batch_size != config.num_samples:
raise ValueError(
"num_samples must be equal to batch_size for Offline scenario"
)
- if config.batch_size is None:
- config.batch_size = config.num_samples
- cmd.extend(["--batch-size", str(config.batch_size)])
+ batch_size = config.batch_size if config.batch_size is not None else config.num_samples
+ cmd.extend(["--batch-size", str(batch_size)])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if config.scenario == "Offline": | |
| if config.num_samples is not None and config.num_samples != config.num_samples: | |
| raise ValueError( | |
| "num_samples must be equal to batch_size for Offline scenario" | |
| ) | |
| if config.batch_size is None: | |
| config.batch_size = config.num_samples | |
| cmd.extend(["--batch-size", str(config.batch_size)]) | |
| if config.scenario == "Offline": | |
| if config.batch_size is not None and config.batch_size != config.num_samples: | |
| raise ValueError( | |
| "num_samples must be equal to batch_size for Offline scenario" | |
| ) | |
| batch_size = config.batch_size if config.batch_size is not None else config.num_samples | |
| cmd.extend(["--batch-size", str(batch_size)]) |
🧰 Tools
🪛 GitHub Check: ruff
[failure] 644-644: Ruff (E501)
auto_tune_vllm/benchmarks/providers.py:644:89: E501 Line too long (91 > 88)
🤖 Prompt for AI Agents
In `@auto_tune_vllm/benchmarks/providers.py` around lines 643 - 650, The current
check mistakenly compares config.num_samples to itself; change the condition to
compare config.batch_size != config.num_samples when config.num_samples is not
None to enforce equality for the "Offline" scenario, and do not mutate the
passed-in BenchmarkConfig: compute a local variable (e.g., batch_size =
config.batch_size if config.batch_size is not None else config.num_samples) and
use that local when calling cmd.extend(["--batch-size", str(batch_size)]) and
when validating equality between config.num_samples and config.batch_size.
| # Example study configuration for MLPerf Harness - Server Scenario on Kubernetes | ||
| # This uses the direct Kubernetes Deployment/Pod backend (no Helm) | ||
| # Simpler than Helm backend - creates Deployments and Services directly |
There was a problem hiding this comment.
Fix inconsistent header comment.
The header states "Server Scenario" but line 46 configures scenario: "Offline". Update the comment to match the actual configuration.
✏️ Proposed fix
-# Example study configuration for MLPerf Harness - Server Scenario on Kubernetes
+# Example study configuration for MLPerf Harness - Offline Scenario on Kubernetes
# This uses the direct Kubernetes Deployment/Pod backend (no Helm)
# Simpler than Helm backend - creates Deployments and Services directly📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Example study configuration for MLPerf Harness - Server Scenario on Kubernetes | |
| # This uses the direct Kubernetes Deployment/Pod backend (no Helm) | |
| # Simpler than Helm backend - creates Deployments and Services directly | |
| # Example study configuration for MLPerf Harness - Offline Scenario on Kubernetes | |
| # This uses the direct Kubernetes Deployment/Pod backend (no Helm) | |
| # Simpler than Helm backend - creates Deployments and Services directly |
🤖 Prompt for AI Agents
In `@examples/study_config_test_gpt_oss.yaml` around lines 1 - 3, Update the top
header comment to match the actual configured scenario: replace the phrase
"Server Scenario" in the header comment with "Offline Scenario" (the
configuration uses scenario: "Offline"); ensure the header consistently
describes the file's purpose and references the scenario key scenario: "Offline"
so the comment and the config stay in sync.
| benchmark_image: "quay.io/rh-ee-aansharm/mlperf-6.0@sha256:ac63fa7a124c6901ce6538f7f1fd52ef6d7c328a1bbd300fbe187d2104901b8b" | ||
| service_type: "ClusterIP" # Service type: ClusterIP, NodePort, or LoadBalancer | ||
| service_port: 8000 # Service port for vLLM server | ||
| kubeconfig: "/Users/aansharm/.kube/config" # Optional: custom kubeconfig path |
There was a problem hiding this comment.
Replace user-specific path with placeholder.
The hardcoded path /Users/aansharm/.kube/config is user-specific and not portable. Use a placeholder or comment to indicate this should be customized.
✏️ Proposed fix
- kubeconfig: "/Users/aansharm/.kube/config" # Optional: custom kubeconfig path
+ kubeconfig: "~/.kube/config" # Optional: custom kubeconfig path (update for your environment)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubeconfig: "/Users/aansharm/.kube/config" # Optional: custom kubeconfig path | |
| kubeconfig: "~/.kube/config" # Optional: custom kubeconfig path (update for your environment) |
🤖 Prompt for AI Agents
In `@examples/study_config_test_gpt_oss.yaml` at line 20, Replace the
user-specific hardcoded kubeconfig value by changing the "kubeconfig" YAML field
to a generic placeholder or commented instruction (e.g., set kubeconfig:
"<PATH_TO_YOUR_KUBECONFIG>" or comment that users should replace with their own
path) so the examples/study_config_test_gpt_oss.yaml is portable and not tied to
a specific user's home directory.
- Reformatted 18 files with ruff format - Fixed 31 auto-fixable linting errors - Remaining: 307 E501 line-too-long warnings (mostly strings/comments)
- Add tensor_parallel_size field to BenchmarkConfig for MLflow tagging - Inject TP from static_params and pass to MLflow via --mlflow-description - Add enable_metrics flag to BenchmarkConfig for metrics collection - Improve trial log management with component-based organization (controller, vllm, benchmark) - Enhance PostgreSQL logging with structured trial_logs table - Update version to 0.1.1 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implement automatic collection of vLLM and benchmark pod logs to PostgreSQL before pod deletion, preventing log loss in Kubernetes deployments. Key changes: - Add pod_log_collector.py module with functions to collect logs from Job and Deployment pods via Kubernetes API - Extend trial_logs table schema with pod_name and container_name columns for pod metadata tracking - Integrate log collection in HelmTrialController and KubernetesTrialController finally blocks (before pod deletion) - Use line-by-line insertion in single transaction for simplicity - Non-blocking: log collection failures do not fail trial cleanup New component types in trial_logs: - 'vllm-pod': Logs from vLLM Deployment pods - 'benchmark-pod': Logs from benchmark Job pods This enables post-mortem debugging of failed trials and log analysis even after pods are deleted by the trial cleanup process. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add result validation tracking (result_valid, result_status user attributes in Optuna) - Add MLflow enhancements: mlflow_port, system tagging, custom mlflow_tags support - Add MLPerf harness debug logging support (log_level, enable_trace flags) - Add num_workers parameter for async request processing in Server scenario - Add mlflow>=2.0.0 dependency to pyproject.toml These changes enable better tracking of VALID/INVALID runs in Optuna dashboard and provide more comprehensive MLflow experiment tracking capabilities.
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.