[llm_d] Work on P/D CPT#126
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThe PR adds direct stdout artifact capture, completion timing, interrupt-aware postprocessing, smoke diagnostics, KServe dry-run deployment, and llm-d benchmark/P/D orchestration with new presets, manifests, tests, and documentation. ChangesCore runtime behavior
Artifact capture
Smoke and KServe workflows
llm-d benchmark and P/D orchestration
Developer and catalog tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
/test fournos llm_d smoke |
|
/test fournos llm_d smoke |
🛑 Execution of
|
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
a94115b to
e503280
Compare
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
|
[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 |
|
/test fournos llm_d smoke |
|
/test fournos llm_d smoke |
🔴 Submission of
|
|
/test fournos llm_d smoke |
🔴 Submission of
|
76f89ea to
94d503b
Compare
|
/test fournos llm_d smoke |
🟢 Execution of
|
🟢 Submission of
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
.claude/settings.json (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope Ruff to the changed file.
These hooks run
ruff ... .after every Python edit, so one change can rewrite unrelated Python files and create noisy or conflicting diffs. Pass"$f"instead, or explicitly document why repository-wide mutation is intended.Suggested adjustment
- ruff format . + ruff format "$f" - ruff check --fix . + ruff check --fix "$f"Also applies to: 14-14, 24-24, 29-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/settings.json at line 9, Update the Python file formatting hooks in the command entries to run Ruff against the changed file variable "$f" instead of the repository-wide "." target. Apply this consistently to all corresponding hooks while preserving the existing Python-file check and error suppression behavior.projects/llm_d/orchestration/test_phase.py (2)
214-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate log statements.
The empty f-string and the two separate log statements can be consolidated into a single formatted log call for cleaner code.
♻️ Proposed refactor
if dry_run: llmisvc_manifest_path = endpoint_url - logger.info(f"Dry-run completed: LLMInferenceService manifest prepared:") - logger.info(llmisvc_manifest_path) + logger.info("Dry-run completed: LLMInferenceService manifest prepared: %s", llmisvc_manifest_path) return llmisvc_manifest_path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm_d/orchestration/test_phase.py` around lines 214 - 218, Consolidate the two logger.info calls in the dry_run branch of the orchestration flow into one formatted log statement that includes the prepared llmisvc_manifest_path, removing the unnecessary empty f-string while preserving the existing return value.
137-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer consistent logger usage.
This file predominantly relies on a locally imported or configured
loggerinstance (e.g.,logger.info,logger.exception), but this line uses theloggingmodule directly. Consider standardizing onlogger.warningfor consistency.♻️ Proposed refactor
if dry_run: - logging.warning("Running in dry-run mode, skipping the rest of the test steps") + logger.warning("Running in dry-run mode, skipping the rest of the test steps") return 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm_d/orchestration/test_phase.py` around lines 137 - 140, In the dry-run branch of test execution, replace the direct logging-module call with the file’s configured logger instance by using logger.warning, while preserving the existing warning message and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.json:
- Line 9: Update the command hook around the jq and Ruff pipeline to remove the
unconditional “2>/dev/null || true” suppression, allowing jq or ruff failures
and missing executables to return a non-zero status. If the hook must remain
best-effort, replace silent suppression with a visible warning while preserving
failure visibility.
In `@projects/core/library/config.py`:
- Around line 548-553: Update the override-loading logic in the override_file
branch to retain the parsed YAML only when it is a dictionary; otherwise, assign
an empty dictionary. Ensure this validation covers falsy and truthy
non-dictionary values before subsequent overrides assignments.
In `@projects/kserve/toolbox/deploy_llmisvc/main.py`:
- Around line 179-180: Replace eager context-attribute defaults in
projects/kserve/toolbox/deploy_llmisvc/main.py at lines 179-180, 216-217, 128,
157, 268, 295, and 332 with nested safe getattr lookups; in the flows at 179-180
and 216-217, return “No service name available” when no name is found. At lines
403-405, use service_description when present and otherwise build the fallback
using a safe nested inference_service_name lookup defaulting to “Unknown”.
In `@projects/llm_d/orchestration/ci.py`:
- Around line 84-89: Update the preset normalization logic around presets_list
to split comma-separated values from every --preset argument, preserving their
order alongside non-comma values. Emit the “Setting preset configuration from
CLI” log consistently whenever preset arguments are provided, including single
values without commas, and log the fully expanded list.
In `@projects/rhoai/library/deploy.py`:
- Line 128: Update the artifact_dirname_suffix argument in the deployment
configuration from "_rhoai" to "rhoai", so runtime.py’s suffix formatting
produces a single underscore in the artifact directory name.
---
Nitpick comments:
In @.claude/settings.json:
- Line 9: Update the Python file formatting hooks in the command entries to run
Ruff against the changed file variable "$f" instead of the repository-wide "."
target. Apply this consistently to all corresponding hooks while preserving the
existing Python-file check and error suppression behavior.
In `@projects/llm_d/orchestration/test_phase.py`:
- Around line 214-218: Consolidate the two logger.info calls in the dry_run
branch of the orchestration flow into one formatted log statement that includes
the prepared llmisvc_manifest_path, removing the unnecessary empty f-string
while preserving the existing return value.
- Around line 137-140: In the dry-run branch of test execution, replace the
direct logging-module call with the file’s configured logger instance by using
logger.warning, while preserving the existing warning message and return
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0268e43-993d-4c64-9c3b-5538b6544ede
📒 Files selected for processing (33)
.claude/settings.jsonprojects/agentic_tools/locust/toolbox/generate_prompts/main.pyprojects/agentic_tools/mcp/toolbox/deploy_mock_servers/main.pyprojects/core/dsl/log.pyprojects/core/dsl/runtime.pyprojects/core/dsl/utils/k8s.pyprojects/core/library/ci.pyprojects/core/library/config.pyprojects/core/library/postprocess.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_smoke_request/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/kserve/toolbox/prepare_hf_model_cache/main.pyprojects/llm_d/CHANGELOG-orchestration.mdprojects/llm_d/PD_TESTING.mdprojects/llm_d/docs/benchmark-isolation.mdprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/cleanup_phase.pyprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/config.d/workloads.yamlprojects/llm_d/orchestration/manifests/deployments/pd-scheduler.yamlprojects/llm_d/orchestration/presets.d/presets.yamlprojects/llm_d/orchestration/runtime_config.pyprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/tests/test_pd_integration.pyprojects/llm_d/tests/test_profiles.pyprojects/llm_d/toolbox/capture_prepare_state/main.pyprojects/mcp_gateway/toolbox/apply_infrastructure/main.pyprojects/mcp_gateway/toolbox/cleanup_platform/main.pyprojects/mcp_gateway/toolbox/install_platform/main.pyprojects/rhoai/library/deploy.py
08e3f87 to
4c2b148
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
projects/llm_d/PD_TESTING.md (1)
86-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to fenced code blocks.
Use
textfor directive, tree, and formula examples (or the most appropriate language) to satisfy Markdown linting and improve rendering.Proposed fix
-``` +```textAlso applies to: 140-151, 185-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm_d/PD_TESTING.md` around lines 86 - 103, Add language identifiers to the fenced code blocks in the deployment profile examples and the other referenced sections, using text for directive, tree, and formula snippets or another appropriate language where applicable. Preserve the example contents and formatting while ensuring every affected fence has an explicit language tag.Source: Linters/SAST tools
projects/llm_d/tests/test_pd_integration.py (1)
93-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefactor this test to exercise the orchestration logic.
This test verifies basic Python iteration and multiplication (2 * 3 * 2 == 12) rather than exercising the project's codebase. Consider refactoring it to apply the actual
pd-matrixpreset and callruntime_config.get_run_specs()(similar totest_3d_matrix_with_multiple_benchmark_keysintest_profiles.py), or remove it if that coverage is already sufficient elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/llm_d/tests/test_pd_integration.py` around lines 93 - 119, Replace the hand-built calculations in test_matrix_expansion_calculation with an integration test that loads/applies the actual pd-matrix preset and invokes runtime_config.get_run_specs(), following the pattern of test_3d_matrix_with_multiple_benchmark_keys in test_profiles.py. Assert the generated RunSpecs and relevant model, deployment-profile, and benchmark combinations, or remove this test if equivalent preset coverage already exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/library/postprocess.py`:
- Around line 179-192: Update the status handling around
run_postprocess_after_test to tolerate a None result when postprocessing is
disabled, treating it as an unsuccessful or otherwise non-postprocessed status
without calling .get on None. In the result is None branch after
_handle_postprocess_failure, remove the bare raise and fall through so the
original test exception propagates through the outer flow instead of being
caught by the inner except Exception handler or chained as a postprocessing
error.
In `@projects/guidellm/toolbox/run_smoke_request/main.py`:
- Around line 239-264: Update the diagnostic capture flow around the pod YAML
and description oc calls to retain each command result, detect nonzero exit
codes, and write the command’s stderr to the corresponding artifact destination
as a fallback. Only return the successful “Captured debug info” message when
both diagnostics succeed; otherwise report the failed diagnostic commands and
their recorded fallback artifacts.
In `@projects/llm_d/orchestration/config.d/runtime.yaml`:
- Line 8: Update the kserve_dry_run setting in the runtime configuration to
false so committed repository defaults perform actual KServe deployments; retain
true only through explicit local or test overrides.
In `@projects/llm_d/orchestration/presets.d/presets.yaml`:
- Around line 175-178: Remove the empty extends key from the pd-flavors preset
while retaining runtime.selected_preset: pd-flavors; do not leave a
commented-out list item or null-valued extends field.
In `@projects/llm_d/orchestration/render_inference_service.py`:
- Around line 33-34: Update P/D detection in the inference service renderer to
use bool(deployment_profile.get("pd_config")), then make the P/D rendering path
consume the prefill and decode settings from pd_config instead of fixed replica
counts and shared tensor-parallel/VLLM arguments. Preserve non-P/D rendering,
and regenerate the affected golden manifest for the ...ptp4-px1-dtp4 profile.
In `@projects/llm_d/PD_TESTING.md`:
- Around line 13-33: Update the “P/D Deployment Profiles” section in
PD_TESTING.md to document the pd-x2-ptp4-px1-dtp4 profile and its pd_config
settings from deployments.yaml, or explicitly clarify that pd-small, pd-medium,
and pd-large are baseline profiles rather than the complete set.
- Around line 139-151: Update the “Artifact Directory Structure” example to use
the runtime-generated run directory names from runtime_config.get_run_specs():
llmd_run for a single run and llmd_run_<profile>_<model>[_<benchmark>] for
matrix runs. Replace the mismatched llmd_test and
prepare_llmd_run_pd_medium_8b_instruct paths, or explicitly document the
additional directory layer if it is intentional.
---
Nitpick comments:
In `@projects/llm_d/PD_TESTING.md`:
- Around line 86-103: Add language identifiers to the fenced code blocks in the
deployment profile examples and the other referenced sections, using text for
directive, tree, and formula snippets or another appropriate language where
applicable. Preserve the example contents and formatting while ensuring every
affected fence has an explicit language tag.
In `@projects/llm_d/tests/test_pd_integration.py`:
- Around line 93-119: Replace the hand-built calculations in
test_matrix_expansion_calculation with an integration test that loads/applies
the actual pd-matrix preset and invokes runtime_config.get_run_specs(),
following the pattern of test_3d_matrix_with_multiple_benchmark_keys in
test_profiles.py. Assert the generated RunSpecs and relevant model,
deployment-profile, and benchmark combinations, or remove this test if
equivalent preset coverage already exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ceaa5ff-d571-457b-8360-fbf02c167c3f
📒 Files selected for processing (40)
.claude/settings.jsonprojects/agentic_tools/locust/toolbox/generate_prompts/main.pyprojects/agentic_tools/mcp/toolbox/deploy_mock_servers/main.pyprojects/core/CHANGELOG-config.mdprojects/core/dsl/log.pyprojects/core/dsl/runtime.pyprojects/core/dsl/utils/k8s.pyprojects/core/library/ci.pyprojects/core/library/config.pyprojects/core/library/postprocess.pyprojects/guidellm/toolbox/run_guidellm_benchmark/main.pyprojects/guidellm/toolbox/run_smoke_request/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/kserve/toolbox/prepare_hf_model_cache/main.pyprojects/llm_d/CHANGELOG-orchestration.mdprojects/llm_d/PD_TESTING.mdprojects/llm_d/docs/benchmark-isolation.mdprojects/llm_d/orchestration/ci.pyprojects/llm_d/orchestration/cleanup_phase.pyprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/config.d/workloads.yamlprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/manifests/deployments/pd-scheduler.yamlprojects/llm_d/orchestration/presets.d/presets.yamlprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/orchestration/runtime_config.pyprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/tests/reference_deployments/deployment-intelligentrouting-tp4-x4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/deployment-pd-x2-ptp4-px1-dtp4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/deployment-simple-tp4-x4/llmisvc.yamlprojects/llm_d/tests/test_deployment_profiles.pyprojects/llm_d/tests/test_pd_integration.pyprojects/llm_d/tests/test_profiles.pyprojects/llm_d/toolbox/capture_prepare_state/main.pyprojects/mcp_gateway/toolbox/apply_infrastructure/main.pyprojects/mcp_gateway/toolbox/cleanup_platform/main.pyprojects/mcp_gateway/toolbox/install_platform/main.pyprojects/rhoai/library/deploy.py
🚧 Files skipped from review as they are similar to previous changes (22)
- projects/core/dsl/log.py
- projects/rhoai/library/deploy.py
- .claude/settings.json
- projects/llm_d/CHANGELOG-orchestration.md
- projects/agentic_tools/mcp/toolbox/deploy_mock_servers/main.py
- projects/llm_d/orchestration/manifests/deployments/pd-scheduler.yaml
- projects/mcp_gateway/toolbox/install_platform/main.py
- projects/mcp_gateway/toolbox/cleanup_platform/main.py
- projects/core/dsl/runtime.py
- projects/agentic_tools/locust/toolbox/generate_prompts/main.py
- projects/llm_d/orchestration/cleanup_phase.py
- projects/llm_d/orchestration/config.d/workloads.yaml
- projects/llm_d/toolbox/capture_prepare_state/main.py
- projects/mcp_gateway/toolbox/apply_infrastructure/main.py
- projects/llm_d/orchestration/config.d/deployments.yaml
- projects/llm_d/orchestration/ci.py
- projects/core/library/ci.py
- projects/kserve/toolbox/prepare_hf_model_cache/main.py
- projects/llm_d/tests/test_profiles.py
- projects/core/dsl/utils/k8s.py
- projects/llm_d/orchestration/test_phase.py
- projects/llm_d/orchestration/runtime_config.py
| try: | ||
| # Capture pod YAML | ||
| oc( | ||
| "get", | ||
| "pod", | ||
| ctx.pod_name, | ||
| "-n", | ||
| args.namespace, | ||
| "-o", | ||
| "yaml", | ||
| check=False, | ||
| stdout_dest=artifacts_dir / f"{ctx.pod_name}.yaml", | ||
| ) | ||
|
|
||
| # Capture pod description | ||
| oc( | ||
| "describe", | ||
| "pod", | ||
| ctx.pod_name, | ||
| "-n", | ||
| args.namespace, | ||
| check=False, | ||
| stdout_dest=artifacts_dir / f"{ctx.pod_name}.description.txt", | ||
| ) | ||
|
|
||
| return f"Captured debug info for pod {ctx.pod_name}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record failed diagnostic commands instead of reporting successful capture.
Both calls use check=False, so a missing/inaccessible pod returns normally with a nonzero exit code. Line 264 then reports success while the artifact may be empty. Check each result and write its stderr to the destination as a fallback.
Proposed fix
- oc(
+ yaml_result = oc(
"get",
"pod",
ctx.pod_name,
"-n",
args.namespace,
"-o",
"yaml",
check=False,
+ log_stdout=False,
stdout_dest=artifacts_dir / f"{ctx.pod_name}.yaml",
)
+ if yaml_result.returncode != 0:
+ write_text(
+ artifacts_dir / f"{ctx.pod_name}.yaml",
+ yaml_result.stderr or "Failed to capture pod YAML",
+ )
- oc(
+ describe_result = oc(
"describe",
"pod",
ctx.pod_name,
"-n",
args.namespace,
check=False,
+ log_stdout=False,
stdout_dest=artifacts_dir / f"{ctx.pod_name}.description.txt",
)
+ if describe_result.returncode != 0:
+ write_text(
+ artifacts_dir / f"{ctx.pod_name}.description.txt",
+ describe_result.stderr or "Failed to describe pod",
+ )📝 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.
| try: | |
| # Capture pod YAML | |
| oc( | |
| "get", | |
| "pod", | |
| ctx.pod_name, | |
| "-n", | |
| args.namespace, | |
| "-o", | |
| "yaml", | |
| check=False, | |
| stdout_dest=artifacts_dir / f"{ctx.pod_name}.yaml", | |
| ) | |
| # Capture pod description | |
| oc( | |
| "describe", | |
| "pod", | |
| ctx.pod_name, | |
| "-n", | |
| args.namespace, | |
| check=False, | |
| stdout_dest=artifacts_dir / f"{ctx.pod_name}.description.txt", | |
| ) | |
| return f"Captured debug info for pod {ctx.pod_name}" | |
| try: | |
| # Capture pod YAML | |
| yaml_result = oc( | |
| "get", | |
| "pod", | |
| ctx.pod_name, | |
| "-n", | |
| args.namespace, | |
| "-o", | |
| "yaml", | |
| check=False, | |
| log_stdout=False, | |
| stdout_dest=artifacts_dir / f"{ctx.pod_name}.yaml", | |
| ) | |
| if yaml_result.returncode != 0: | |
| write_text( | |
| artifacts_dir / f"{ctx.pod_name}.yaml", | |
| yaml_result.stderr or "Failed to capture pod YAML", | |
| ) | |
| # Capture pod description | |
| describe_result = oc( | |
| "describe", | |
| "pod", | |
| ctx.pod_name, | |
| "-n", | |
| args.namespace, | |
| check=False, | |
| log_stdout=False, | |
| stdout_dest=artifacts_dir / f"{ctx.pod_name}.description.txt", | |
| ) | |
| if describe_result.returncode != 0: | |
| write_text( | |
| artifacts_dir / f"{ctx.pod_name}.description.txt", | |
| describe_result.stderr or "Failed to describe pod", | |
| ) | |
| return f"Captured debug info for pod {ctx.pod_name}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/guidellm/toolbox/run_smoke_request/main.py` around lines 239 - 264,
Update the diagnostic capture flow around the pod YAML and description oc calls
to retain each command result, detect nonzero exit codes, and write the
command’s stderr to the corresponding artifact destination as a fallback. Only
return the successful “Captured debug info” message when both diagnostics
succeed; otherwise report the failed diagnostic commands and their recorded
fallback artifacts.
| benchmark_key: null | ||
| job_name: null | ||
| namespace_override: null | ||
| kserve_dry_run: true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify if kserve_dry_run: true should be the repository default.
Setting kserve_dry_run: true in the committed configuration makes dry-run mode the default behavior, meaning actual KServe deployments will be skipped unless this is explicitly overridden. If this was just used for local or PR testing (as mentioned in the PR objectives), consider reverting it to false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/llm_d/orchestration/config.d/runtime.yaml` at line 8, Update the
kserve_dry_run setting in the runtime configuration to false so committed
repository defaults perform actual KServe deployments; retain true only through
explicit local or test overrides.
| pd-flavors: | ||
| extends: | ||
| # - gpt-oss | ||
| runtime.selected_preset: pd-flavors |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove empty extends key.
Commenting out the only list item under extends leaves the key evaluating to null (None). If the configuration loading logic expects this to be a list and attempts to iterate over it, it will cause a runtime crash (TypeError: 'NoneType' object is not iterable).
🐛 Proposed fix
pd-flavors:
- extends:
- # - gpt-oss
runtime.selected_preset: pd-flavors📝 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.
| pd-flavors: | |
| extends: | |
| # - gpt-oss | |
| runtime.selected_preset: pd-flavors | |
| pd-flavors: | |
| runtime.selected_preset: pd-flavors |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/llm_d/orchestration/presets.d/presets.yaml` around lines 175 - 178,
Remove the empty extends key from the pd-flavors preset while retaining
runtime.selected_preset: pd-flavors; do not leave a commented-out list item or
null-valued extends field.
| # Check if this is a P/D deployment | ||
| is_pd_deployment = "pd_config" in deployment_profile |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Render P/D role settings from pd_config.
Detection differs from runtime_config.is_pd_deployment(), and the P/D renderer never consumes pd_config: both replica counts are fixed at 2, while tensor parallelism and VLLM arguments are shared. Consequently, role-specific profiles render incorrectly—the ...ptp4-px1-dtp4 reference already emits prefill.replicas: 2.
Use bool(deployment_profile.get("pd_config")) and populate each role from its P/D configuration, then update the golden manifest.
Also applies to: 255-265, 281-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/llm_d/orchestration/render_inference_service.py` around lines 33 -
34, Update P/D detection in the inference service renderer to use
bool(deployment_profile.get("pd_config")), then make the P/D rendering path
consume the prefill and decode settings from pd_config instead of fixed replica
counts and shared tensor-parallel/VLLM arguments. Preserve non-P/D rendering,
and regenerate the affected golden manifest for the ...ptp4-px1-dtp4 profile.
| ### Artifact Directory Structure | ||
| ``` | ||
| ARTIFACT_DIR/ | ||
| ├── llmd_test/ | ||
| │ ├── __test_labels__.yaml | ||
| │ ├── artifacts/ | ||
| │ │ ├── pd-scheduler-config.yaml # P/D scheduler manifest | ||
| │ │ ├── llminferenceservice.yaml | ||
| │ │ ├── endpoint.url | ||
| │ │ └── results/ | ||
| │ │ └── benchmark_short/ | ||
| ├── prepare_llmd_run_pd_medium_8b_instruct/ # P/D-specific prepare artifacts | ||
| └── ... |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the artifact example with runtime-generated names.
runtime_config.get_run_specs() uses llmd_run for a single run and llmd_run_<profile>_<model>[_<benchmark>] for matrices. The llmd_test/ and prepare_llmd_run_pd_medium_8b_instruct/ paths shown here do not match that contract; update the example or explain the additional directory layer.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 140-140: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/llm_d/PD_TESTING.md` around lines 139 - 151, Update the “Artifact
Directory Structure” example to use the runtime-generated run directory names
from runtime_config.get_run_specs(): llmd_run for a single run and
llmd_run_<profile>_<model>[_<benchmark>] for matrix runs. Replace the mismatched
llmd_test and prepare_llmd_run_pd_medium_8b_instruct paths, or explicitly
document the additional directory layer if it is intentional.
8a1b354 to
188cf4b
Compare
…en the Pod doesn't start
…launching a command
before the env module is initialized
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
…build the llmisvc src
2 participants
Summary by CodeRabbit