[llm-d] Start testing P/D on CKS - #136
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates core execution utilities, adds asynchronous Fournos job submission, introduces a service-account kubeconfig workflow, and reworks llm-d deployment profiles, P/D rendering, scheduler configuration, namespace handling, Kueue integration, runtime inputs, and deployment validation. ChangesCore runtime and toolbox workflows
llm-d deployment orchestration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TestPhase
participant RuntimeConfig
participant RenderInferenceService
participant SchedulerConfig
participant LLMInferenceService
TestPhase->>RuntimeConfig: resolve deployment profile and FROM_NAME values
RuntimeConfig->>SchedulerConfig: load router template and EPP configuration
TestPhase->>RenderInferenceService: provide resolved profile and model inputs
RenderInferenceService->>LLMInferenceService: emit annotated manifest
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
projects/fournos_launcher/toolbox/submit_and_wait/main.py (2)
219-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated status-polling logic between
wait_for_job_to_resolveandwait_for_job_completion.Both functions repeat the same
oc get fournosjob ... -o jsonpath="{.status.phase}"call, "not found" handling, and stderr-retry logic. Consider extracting a shared helper (e.g.,_get_job_phase(args, ctx)) to reduce duplication.Also applies to: 304-364
🤖 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/fournos_launcher/toolbox/submit_and_wait/main.py` around lines 219 - 289, Extract the duplicated status polling from wait_for_job_to_resolve and wait_for_job_completion into a shared helper such as _get_job_phase(args, ctx). Keep the existing oc command, not-found FournosJobFailureError handling, and temporary-error retry behavior in the helper, then update both functions to consume its result while preserving their phase-specific handling.
68-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring not updated for new
waitparameter.📝 Proposed fix
clusterless: Whether to enable clusterless mode (makes cluster_name optional, forces exclusive=False) (default: False) + wait: Whether to wait for job completion after submission; if False, returns after launch (default: True)🤖 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/fournos_launcher/toolbox/submit_and_wait/main.py` around lines 68 - 90, Update the submit-and-wait function docstring to document the existing wait parameter, including its boolean meaning and default value, alongside the other Args entries.projects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yaml (1)
4-7: 📐 Maintainability & Code Quality | 🔵 TrivialTracked TODO: profile diverges from upstream precise-prefix-cache routing.
This config keeps the older scorer-only setup instead of the upstream producer + ZMQ KV-events approach. Want me to open a tracking issue so this alignment isn't lost?
🤖 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/manifests/scheduler_config/precise-prefix-cache.yaml` around lines 4 - 7, Update the precise-prefix-cache profile to match the upstream routing guide by replacing the scorer-only configuration with precise-prefix-cache-producer plus prefix-cache-scorer, and configure vLLM KV events to publish through ZMQ using KV_EVENTS_ENDPOINT=tcp://*:5556. Remove the obsolete TODO once the profile is aligned.projects/llm_d/orchestration/runtime_config.py (1)
590-605: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDrop the stale
namespace_overrideread fromactivate_run_spec.runtime.namespace_overrideis no longer inruntime.yaml, and_get_runtime_value()returnsNonefor missing keys, so this entry is dead state.🤖 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/runtime_config.py` around lines 590 - 605, Remove the namespace_override entry from the saved state in activate_run_spec. Keep the existing model_name, deployment_profile, and benchmark_key preservation and restoration unchanged.projects/llm_d/docs/PD_TESTING.md (1)
284-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNaming doesn't match actual config schema.
This describes
deployments.defaults.vllm_extra_args/ profilevllm_extra_args(flat, underscored), but the actual schema uses nestedvllm_extra: { args: [...] }(seedeployments.pd.vllm_extra.argsused viaconfig.project.get_config(...)inrender_inference_service.py). Worth aligning the doc with the real key names, especially since the profile-level lookup for this exact key has a separate bug (seerender_inference_service.pyreview).🤖 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/docs/PD_TESTING.md` around lines 284 - 289, Update the VLLM Configuration section to use the actual nested schema names: describe base and profile arguments through vllm_extra.args, matching the existing deployments.pd.vllm_extra.args form and config.project.get_config(...) usage. Remove references to the nonexistent flat vllm_extra_args keys while preserving the documented argument-combination 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 `@projects/core/library/config.py`:
- Around line 213-215: Update the extends handling around apply_preset so a
single string value is normalized into a one-item iterable before iteration,
while preserving the existing empty/null behavior and list handling. Ensure
values such as “my-preset” are passed once to self.apply_preset rather than
iterated character by character.
In `@projects/core/library/env.py`:
- Around line 113-118: Update the forge_last symlink management around
forge_last_link to eliminate the check-then-act race: create the new symlink via
a temporary sibling path and atomically replace forge_last, or otherwise handle
concurrent FileNotFoundError/FileExistsError safely. Preserve the final target
as artifact_dir.name and avoid crashing when concurrent executions update the
link.
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Around line 258-264: Update the status-handling branch in the submit-and-wait
flow for status values "Stopping" or "" so its logger.info message reflects the
immediate permanent failure and no longer says to keep waiting. Preserve the
existing FournosJobFailureError behavior and leave the separate Resolving branch
unchanged.
- Around line 213-216: Remove the redundant unreachable wait check from
check_early_return, since submit_fournos_job already returns EarlyReturn when
args.wait is false. Consolidate the gating so the later task only handles the
wait=True path and returns the successful submission result without another
args.wait branch.
In `@projects/llm_d/docs/PD_TESTING.md`:
- Line 107: Add language identifiers to every fenced code block in PD_TESTING.md
flagged by MD040, using text for CLI, directive, and directory-tree snippets
while preserving the existing block contents.
In `@projects/llm_d/orchestration/render_inference_service.py`:
- Around line 273-318: Align the prefill tensor-parallelism handling in the
VLLM-args branch with the fallback behavior already used in the resources
branch. Update the extraction around `_build_vllm_additional_args` to catch
`ValueError` and use the unmodified `deployment_profile` when extraction fails,
so rendering continues consistently with the `_build_serving_resources` path.
- Around line 66-89: Remove the dead `name = inference_service["name"]` and
initial metadata-name assignment from the manifest setup. In
`_render_standard_deployment` and `_render_pd_deployment`, handle the optional
`deployment_profile_name` explicitly so it is never interpolated as
`"None"`—either require the parameter at the rendering boundary or apply the
intended fallback/error behavior consistently with the guarded annotation logic.
- Around line 244-255: Update the P/D rendering flow around
_build_pd_pod_template so both spec.prefill.template and spec.template merge
their container changes into the existing base pod templates, preserving fields
such as tolerations and liveness/readiness probes. Replace the global
get_prefill_pod_count() and get_decode_pod_count() calls with replica counts
read from the passed deployment_profile["pd_config"], so locally overridden
profiles remain consistent with the rendered templates.
- Around line 164-181: Update _build_vllm_additional_args to retrieve
deployment-specific arguments through the nested vllm_extra then args mappings,
using empty dictionaries as fallbacks. Preserve the existing default-argument
lookup and combination behavior.
In `@projects/llm_d/orchestration/runtime_config.py`:
- Line 422: Remove the stray “# REMOVE THIS” marker from the affected code in
runtime_config.py; if the surrounding function is intentionally slated for
removal, delete that function instead, ensuring no placeholder remains before
merge.
In `@projects/llm_d/tests/test_deployment_profiles.py`:
- Around line 212-214: Update the comment above the profile_name check to
describe that a missing annotation causes ValueError to be raised, removing the
inaccurate fallback-to-“unknown” wording. Keep the existing ValueError behavior
unchanged.
In `@projects/llm_d/tests/test_pd_integration.py`:
- Around line 120-134: Update the mock configuration in
test_pd_runtime_config_functions so scheduler_manifest no longer references the
deleted manifests/deployments/pd-scheduler.yaml path; use the current
manifests/scheduler_config/* EPP layout while preserving the rest of the test
configuration.
- Around line 20-90: Update test_pd_deployment_profiles_exist to validate the
shipped template__pd and FROM_NAME-based profile references instead of
pd-small/pd-medium/pd-large, including the new profile-driven configuration
shape. Replace test_pd_presets_exist’s legacy preset names and matrix assertions
with checks for the current presets.yaml entries and template__pd/FROM_NAME
naming flow. Rewrite test_pd_scheduler_manifest_exists to validate the router
template and scheduler_config/*.yaml EPP mechanism rather than the removed
manifests/deployments/pd-scheduler.yaml, preserving checks for prefill/decode
scheduler configuration.
---
Nitpick comments:
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Around line 219-289: Extract the duplicated status polling from
wait_for_job_to_resolve and wait_for_job_completion into a shared helper such as
_get_job_phase(args, ctx). Keep the existing oc command, not-found
FournosJobFailureError handling, and temporary-error retry behavior in the
helper, then update both functions to consume its result while preserving their
phase-specific handling.
- Around line 68-90: Update the submit-and-wait function docstring to document
the existing wait parameter, including its boolean meaning and default value,
alongside the other Args entries.
In `@projects/llm_d/docs/PD_TESTING.md`:
- Around line 284-289: Update the VLLM Configuration section to use the actual
nested schema names: describe base and profile arguments through
vllm_extra.args, matching the existing deployments.pd.vllm_extra.args form and
config.project.get_config(...) usage. Remove references to the nonexistent flat
vllm_extra_args keys while preserving the documented argument-combination
behavior.
In
`@projects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yaml`:
- Around line 4-7: Update the precise-prefix-cache profile to match the upstream
routing guide by replacing the scorer-only configuration with
precise-prefix-cache-producer plus prefix-cache-scorer, and configure vLLM KV
events to publish through ZMQ using KV_EVENTS_ENDPOINT=tcp://*:5556. Remove the
obsolete TODO once the profile is aligned.
In `@projects/llm_d/orchestration/runtime_config.py`:
- Around line 590-605: Remove the namespace_override entry from the saved state
in activate_run_spec. Keep the existing model_name, deployment_profile, and
benchmark_key preservation and restoration unchanged.
🪄 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: 9760f122-371b-4cf7-a190-cbeb5234139f
📒 Files selected for processing (23)
projects/core/library/config.pyprojects/core/library/env.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/llm_d/CHANGELOG-orchestration.mdprojects/llm_d/docs/PD_TESTING.mdprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/config.d/project.yamlprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yamlprojects/llm_d/orchestration/manifests/llminferenceservice.yamlprojects/llm_d/orchestration/manifests/scheduler_config/__router_template__.yamlprojects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yamlprojects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.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/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-simple-tp4-x4/llmisvc.yamlprojects/llm_d/tests/test_deployment_profiles.pyprojects/llm_d/tests/test_pd_integration.py
💤 Files with no reviewable changes (3)
- projects/llm_d/orchestration/config.d/project.yaml
- projects/llm_d/orchestration/config.d/platform.yaml
- projects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yaml
| if not args.wait: | ||
| return EarlyReturn(f"Submitted FournosJob: {ctx.final_job_name} (wait=False)") | ||
|
|
||
| return f"Successfully submitted FOURNOS job: {ctx.final_job_name}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm task execution ordering semantics (top-to-bottom by definition?) in the DSL runtime.
fd -e py . projects/core/dsl -x cat -n {}Repository: openshift-psap/forge
Length of output: 50377
check_early_return is redundant and unreachable. submit_fournos_job already returns EarlyReturn when wait=False, so this later task never runs on that path and its own if not args.wait branch can’t fire when wait=True. Consolidate the gating in one place.
🤖 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/fournos_launcher/toolbox/submit_and_wait/main.py` around lines 213 -
216, Remove the redundant unreachable wait check from check_early_return, since
submit_fournos_job already returns EarlyReturn when args.wait is false.
Consolidate the gating so the later task only handles the wait=True path and
returns the successful submission result without another args.wait branch.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/core/library/postprocess.py (1)
251-255: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate
run_postprocess_after_testto return the status mapping type.
run_postprocess_after_testreturns a dict on both paths, so-> Noneis misleading and will trip type checking. Change it todict[str, Any].🤖 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/core/library/postprocess.py` around lines 251 - 255, Update the return annotation of run_postprocess_after_test from None to dict[str, Any], matching the dictionary returned by both execution paths.projects/llm_d/orchestration/runtime_config.py (1)
556-577: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
artifact_dirnamebecomes the literal string"llmd__None"when no benchmark key is set.When
runtime.benchmark_keyis unset,benchmark_entries = [(None, None)]andbench_keyisNone, soartifact_dirname=f"llmd__{bench_key}"renders as"llmd__None". This is reachable today: the newcks-pdpreset (projects/llm_d/orchestration/presets.d/cks.yaml) extendscks+gpt-oss, and neither setsruntime.benchmark_key.🐛 Proposed fix
- artifact_dirname=f"llmd__{bench_key}", + artifact_dirname=f"llmd__{bench_key}" if bench_key else "llmd",🤖 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/runtime_config.py` around lines 556 - 577, Update the RunSpec construction in the combination loop so artifact_dirname uses a valid non-benchmark artifact name when bench_key is None instead of interpolating None into "llmd__None"; preserve the existing benchmark-specific naming for configured benchmark keys and align the fallback with the project’s established artifact naming convention.
🧹 Nitpick comments (3)
projects/llm_d/tests/test_deployment_profiles.py (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused constant.
PROJECT_ORCHESTRATION_DIRis defined here but is never used in this test script.♻️ Proposed refactor
-PROJECT_ORCHESTRATION_DIR = Path(__file__).resolve().parents[1] / "orchestration"🤖 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_deployment_profiles.py` at line 14, Remove the unused PROJECT_ORCHESTRATION_DIR constant from the test script, leaving the remaining deployment profile test setup unchanged.projects/llm_d/orchestration/runtime_config.py (1)
220-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment describes a legacy-fallback path that doesn't exist.
The comment "Fallback to legacy behavior if template doesn't exist" doesn't match the code, which simply raises
FileNotFoundErrorwith no fallback. Either implement the described fallback or drop the misleading comment.🤖 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/runtime_config.py` around lines 220 - 246, Remove the misleading “Fallback to legacy behavior” comment in _load_scheduler_with_epp_config, since the missing router template path currently raises FileNotFoundError without performing any fallback.projects/llm_d/orchestration/presets.d/cks.yaml (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded personal namespace (
kpouget-dev) in a shared preset file.This preset (usable by anyone applying
cks/cks-pd) hardcodes a personal dev namespace name. Sinceruntime.namespaceno longer has an auto-derivation fallback (seeplatform.yaml/runtime_config.pychanges), any other user applying this preset unmodified will attempt to deploy intokpouget-dev, causing permission/collision issues on a shared CKS cluster.Consider parameterizing the namespace (e.g., via an environment variable or a placeholder that must be overridden) before this preset is relied upon by others.
🤖 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/cks.yaml` around lines 3 - 9, Replace the hardcoded runtime.namespace value in the cks preset with the repository’s supported parameter, environment variable, or explicit override placeholder, ensuring users must provide their own namespace when applying the preset and preserving the existing Kueue settings.
🤖 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/llm_d/orchestration/config.d/platform.yaml`:
- Around line 8-11: Remove the stray trailing double quote from the
forge.openshift.io/project label under namespace.labels so its value is exactly
llm_d. Leave the app.kubernetes.io/managed-by label and surrounding YAML
structure unchanged.
---
Outside diff comments:
In `@projects/core/library/postprocess.py`:
- Around line 251-255: Update the return annotation of
run_postprocess_after_test from None to dict[str, Any], matching the dictionary
returned by both execution paths.
In `@projects/llm_d/orchestration/runtime_config.py`:
- Around line 556-577: Update the RunSpec construction in the combination loop
so artifact_dirname uses a valid non-benchmark artifact name when bench_key is
None instead of interpolating None into "llmd__None"; preserve the existing
benchmark-specific naming for configured benchmark keys and align the fallback
with the project’s established artifact naming convention.
---
Nitpick comments:
In `@projects/llm_d/orchestration/presets.d/cks.yaml`:
- Around line 3-9: Replace the hardcoded runtime.namespace value in the cks
preset with the repository’s supported parameter, environment variable, or
explicit override placeholder, ensuring users must provide their own namespace
when applying the preset and preserving the existing Kueue settings.
In `@projects/llm_d/orchestration/runtime_config.py`:
- Around line 220-246: Remove the misleading “Fallback to legacy behavior”
comment in _load_scheduler_with_epp_config, since the missing router template
path currently raises FileNotFoundError without performing any fallback.
In `@projects/llm_d/tests/test_deployment_profiles.py`:
- Line 14: Remove the unused PROJECT_ORCHESTRATION_DIR constant from the test
script, leaving the remaining deployment profile test setup unchanged.
🪄 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: dece3f7e-296a-4962-9816-39d81757ef7f
📒 Files selected for processing (29)
projects/core/library/env.pyprojects/core/library/postprocess.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/kserve/toolbox/prepare_hf_model_cache/main.pyprojects/llm_d/CHANGELOG-orchestration.mdprojects/llm_d/docs/PD_TESTING.mdprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/config.d/project.yamlprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yamlprojects/llm_d/orchestration/manifests/llminferenceservice.yamlprojects/llm_d/orchestration/manifests/scheduler_config/__router_template__.yamlprojects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yamlprojects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yamlprojects/llm_d/orchestration/phase_inputs.pyprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/orchestration/presets.d/cks.yamlprojects/llm_d/orchestration/presets.d/cpt.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/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-simple-tp4-x4/llmisvc.yamlprojects/llm_d/tests/test_deployment_profiles.pyprojects/llm_d/tests/test_profiles.py
💤 Files with no reviewable changes (5)
- projects/llm_d/orchestration/config.d/project.yaml
- projects/llm_d/orchestration/prepare_phase.py
- projects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yaml
- projects/kserve/toolbox/prepare_hf_model_cache/main.py
- projects/llm_d/orchestration/phase_inputs.py
🚧 Files skipped from review as they are similar to previous changes (12)
- projects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yaml
- projects/llm_d/orchestration/manifests/llminferenceservice.yaml
- projects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yaml
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yaml
- projects/llm_d/orchestration/config.d/runtime.yaml
- projects/llm_d/orchestration/manifests/scheduler_config/router_template.yaml
- projects/llm_d/orchestration/config.d/deployments.yaml
- projects/llm_d/CHANGELOG-orchestration.md
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yaml
- projects/core/library/env.py
- projects/fournos_launcher/toolbox/submit_and_wait/main.py
- projects/llm_d/orchestration/render_inference_service.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
projects/llm_d/orchestration/test_phase.py (1)
34-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the enabled Kueue path.
The existing profile tests force
runtime.kserve.dry_run=Trueinprojects/llm_d/tests/test_deployment_profiles.py:43-60, soensure_kueue_local_queue()is never exercised. Add a focused test that mocksocand verifies the rendered name, namespace, andclusterQueuefields.🤖 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 34 - 69, Add a focused test for ensure_kueue_local_queue that enables Kueue, supplies a template manifest, and mocks oc so no real command runs. Assert the rendered manifest passed to write_yaml contains the configured queue name, runtime namespace, and matching spec.clusterQueue, and verify the apply path is invoked.
🤖 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/kserve/toolbox/deploy_llmisvc/main.py`:
- Around line 403-416: Update try_resolve_endpoint_url to parse the address URL
and inspect its authority/netloc rather than the final path segment. When no
port is present, append 8000 to the parsed authority while preserving paths,
schemes, and IPv6 host handling; leave URLs with an existing parsed port
unchanged.
- Around line 122-135: Update the pod lookup via oc in the surrounding
deployment-wait logic to request machine-readable output with -o name instead of
--no-headers, and only treat blank stdout as “no pods found” when
result.returncode == 0. Preserve retry behavior for command failures or
non-empty pod output.
---
Nitpick comments:
In `@projects/llm_d/orchestration/test_phase.py`:
- Around line 34-69: Add a focused test for ensure_kueue_local_queue that
enables Kueue, supplies a template manifest, and mocks oc so no real command
runs. Assert the rendered manifest passed to write_yaml contains the configured
queue name, runtime namespace, and matching spec.clusterQueue, and verify the
apply path is invoked.
🪄 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: b02c5dc7-cb2f-4bbc-b8ec-1cad8bc35652
📒 Files selected for processing (10)
projects/core/dsl/task.pyprojects/core/dsl/utils/k8s.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/manifests/kueue-localqueue.yamlprojects/llm_d/orchestration/presets.d/cks.yamlprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/orchestration/test_phase.py
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/llm_d/orchestration/config.yaml
- projects/llm_d/orchestration/presets.d/cks.yaml
- projects/llm_d/orchestration/config.d/runtime.yaml
- projects/llm_d/orchestration/render_inference_service.py
|
/test fournos llm_d cks-pd |
🔴 Submission of
|
|
/test fournos llm_d cks-pd |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d cks-pd |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/llm_d/orchestration/presets.d/cks.yaml`:
- Around line 20-23: Update the runtime.deployment_profile setting in the cks
preset to enable an actual P/D deployment profile, replacing the active
simple-tp4-x4 value with the intended P/D candidate and ensuring it is not
commented out.
In `@projects/llm_d/tests/test_deployment_profiles.py`:
- Around line 70-76: Add a finite timeout to the subprocess.run invocation in
the deployment-profile test helper, using the existing CI test conventions if
available. Ensure a stalled ci.py process raises a timeout failure and does not
block pytest indefinitely.
- Around line 102-104: Update the test around the deployment iteration in
test_deployment_profiles.py to collect the generated profile-name set and
compare it with the profile names discovered from all deployment-*/llmisvc.yaml
reference directories under cpt-reference-flavors before validating individual
deployments. Assert the sets are equal so missing reference profiles fail the
test, while preserving the existing per-deployment checks.
🪄 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: 3121e174-f7d0-4e46-b9c7-3c13fa22931a
📒 Files selected for processing (35)
projects/core/dsl/task.pyprojects/core/dsl/utils/k8s.pyprojects/core/library/config.pyprojects/core/library/env.pyprojects/core/library/postprocess.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/kserve/toolbox/prepare_hf_model_cache/main.pyprojects/llm_d/CHANGELOG-orchestration.mdprojects/llm_d/docs/PD_TESTING.mdprojects/llm_d/orchestration/config.d/deployments.yamlprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/config.d/project.yamlprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yamlprojects/llm_d/orchestration/manifests/kueue-localqueue.yamlprojects/llm_d/orchestration/manifests/llminferenceservice.yamlprojects/llm_d/orchestration/manifests/scheduler_config/__router_template__.yamlprojects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yamlprojects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yamlprojects/llm_d/orchestration/phase_inputs.pyprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/orchestration/presets.d/cks.yamlprojects/llm_d/orchestration/presets.d/cpt.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/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yamlprojects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-simple-tp4-x4/llmisvc.yamlprojects/llm_d/tests/test_deployment_profiles.pyprojects/llm_d/tests/test_profiles.pyprojects/llm_d/toolbox/cleanup_test_resources/main.py
💤 Files with no reviewable changes (5)
- projects/llm_d/orchestration/config.d/project.yaml
- projects/llm_d/orchestration/prepare_phase.py
- projects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yaml
- projects/kserve/toolbox/prepare_hf_model_cache/main.py
- projects/llm_d/orchestration/phase_inputs.py
🚧 Files skipped from review as they are similar to previous changes (24)
- projects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yaml
- projects/llm_d/orchestration/presets.d/cpt.yaml
- projects/llm_d/orchestration/manifests/kueue-localqueue.yaml
- projects/llm_d/orchestration/manifests/llminferenceservice.yaml
- projects/llm_d/orchestration/config.d/runtime.yaml
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-pd-d.x2-p.tp1-d.tp4-p.x8/llmisvc.yaml
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-simple-tp4-x4/llmisvc.yaml
- projects/llm_d/orchestration/config.d/platform.yaml
- projects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yaml
- projects/core/dsl/utils/k8s.py
- projects/core/library/config.py
- projects/llm_d/CHANGELOG-orchestration.md
- projects/core/dsl/task.py
- projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.yaml
- projects/llm_d/orchestration/config.d/deployments.yaml
- projects/core/library/postprocess.py
- projects/llm_d/orchestration/test_phase.py
- projects/core/library/env.py
- projects/kserve/toolbox/deploy_llmisvc/main.py
- projects/llm_d/orchestration/render_inference_service.py
- projects/fournos_launcher/toolbox/submit_and_wait/main.py
- projects/llm_d/orchestration/runtime_config.py
- projects/llm_d/orchestration/presets.d/presets.yaml
- projects/llm_d/tests/test_profiles.py
🔴 Execution of
|
🔴 Submission of
|
a91c7d6 to
93a156e
Compare
|
/test fournos llm_d cks-pd |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos llm_d cks-pd |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/core/library/export.py (1)
727-741: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTest/post-processing status is skipped when there are no step logs.
_build_test_status_section(status)is only invoked insideif step_log_links:, butstep_log_linkscan be empty even whenstatus["test_phase"]is populated (e.g. an early failure before any step directory has arun.log). That's exactly the scenario where surfacing test/post-processing status matters most, yet the notification silently omits it.♻️ Proposed fix
if step_log_links: notification_parts.append("") notification_parts.append("**Test Logs**") notification_parts.extend(step_log_links) - # Add distinct test and post-processing status right under Test Logs - test_status_section = _build_test_status_section(status) - if test_status_section: - notification_parts.append("") - notification_parts.extend(test_status_section) + # Add distinct test and post-processing status right under Test Logs + test_status_section = _build_test_status_section(status) + if test_status_section: + notification_parts.append("") + notification_parts.extend(test_status_section) if postprocess_status_links:🤖 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/core/library/export.py` around lines 727 - 741, Move the _build_test_status_section(status) invocation and its notification appends out of the step_log_links conditional so test/post-processing status is included whenever available, even when no step logs exist. Preserve the existing Test Logs heading and step_log_links behavior, while ensuring the status section remains positioned beneath Test Logs when logs are present and still appears appropriately when they are absent.
🧹 Nitpick comments (1)
projects/fournos_launcher/toolbox/submit_and_wait/main.py (1)
68-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
waitparameter.The
Args:docstring section wasn't updated for the newwait: bool = Trueparameter.♻️ Proposed fix
clusterless: Whether to enable clusterless mode (makes cluster_name optional, forces exclusive=False) (default: False) + wait: Whether to wait for the job to resolve and complete before returning (default: True)🤖 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/fournos_launcher/toolbox/submit_and_wait/main.py` around lines 68 - 90, Update the Args section of the submit-and-wait function docstring to document the wait parameter, including that it controls whether completion is awaited and defaults to True. Preserve the existing documentation for all other parameters.
🤖 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/cluster/toolbox/create_sa_kubeconfig/main.py`:
- Around line 86-89: Update the validation in the main argument-processing flow
to check the resolved ctx.cluster_role_binding_name, established by the
defaulting logic, instead of the raw args.cluster_role_binding_name. Ensure
--cluster-role without an explicit binding name uses the documented
<sa_name>-binding default while still rejecting cases where the resolved binding
name is absent.
- Around line 289-297: The kubeconfig artifact write must always remain within
the artifacts directory and support nested filenames. Update the artifact path
handling near kubeconfig_path to sanitize or reduce ctx.kubeconfig_file to a
safe artifact name, create any required artifact parent directories before
writing, and keep the separate current_dir_path output behavior unchanged.
- Around line 257-259: Update _build_cluster_config to remove
insecure-skip-tls-verify and emit certificate-authority-data from the
ServiceAccount token secret’s ca.crt, encoding it as required by kubeconfig.
Validate that the CA is present and fail clearly when it is unavailable, while
preserving the existing API server value.
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Around line 219-244: Update wait_for_job_to_resolve so an initial “not found”
response is treated as a retryable status-check failure rather than immediately
raising FournosJobFailureError. Preserve the existing retry behavior and
30-attempt/10-second retry budget, while retaining permanent failure handling
for not-found responses only once the job’s existence has been established,
consistent with wait_for_job_completion.
In `@projects/kserve/toolbox/prepare_hf_model_cache/main.py`:
- Around line 290-324: Update wait_for_pods_running to treat a pod phase of
Succeeded as a completed successful state, returning immediately instead of
retrying; preserve the existing Running behavior and allow wait_for_download to
perform the job-level success check afterward.
In `@projects/llm_d/orchestration/preflight_phase.py`:
- Around line 36-41: Update the missing-CRD handling in the preflight function
to guard the ci_lib.add_notification_file call against exceptions, including
failures before its internal try block, and handle a None return without
allowing notification persistence to change the documented False result. Always
return False after logging the CRD failure, regardless of notification
persistence outcome.
In `@projects/llm_d/orchestration/test_phase.py`:
- Around line 161-162: Move the ensure_kueue_local_queue() call into the with
env.NextArtifactDir("llmd_test") context in the llmd_test setup, ensuring it
writes the LocalQueue manifest to that inner artifact directory alongside the
other test artifacts.
---
Outside diff comments:
In `@projects/core/library/export.py`:
- Around line 727-741: Move the _build_test_status_section(status) invocation
and its notification appends out of the step_log_links conditional so
test/post-processing status is included whenever available, even when no step
logs exist. Preserve the existing Test Logs heading and step_log_links behavior,
while ensuring the status section remains positioned beneath Test Logs when logs
are present and still appears appropriately when they are absent.
---
Nitpick comments:
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Around line 68-90: Update the Args section of the submit-and-wait function
docstring to document the wait parameter, including that it controls whether
completion is awaited and defaults to True. Preserve the existing documentation
for all other parameters.
🪄 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: d2475638-dbbb-43d0-a433-188656ec538c
📒 Files selected for processing (22)
projects/cluster/toolbox/create_sa_kubeconfig/main.pyprojects/core/dsl/runtime.pyprojects/core/dsl/task.pyprojects/core/dsl/utils/k8s.pyprojects/core/library/export.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/kserve/toolbox/capture_llmisvc_state/main.pyprojects/kserve/toolbox/deploy_llmisvc/main.pyprojects/kserve/toolbox/prepare_hf_model_cache/main.pyprojects/llm_d/orchestration/config.d/platform.yamlprojects/llm_d/orchestration/config.d/runtime.yamlprojects/llm_d/orchestration/config.yamlprojects/llm_d/orchestration/manifests/kueue-localqueue.yamlprojects/llm_d/orchestration/phase_inputs.pyprojects/llm_d/orchestration/preflight_phase.pyprojects/llm_d/orchestration/prepare_phase.pyprojects/llm_d/orchestration/presets.d/cks.yamlprojects/llm_d/orchestration/render_inference_service.pyprojects/llm_d/orchestration/test_phase.pyprojects/llm_d/tests/test_deployment_profiles.pyprojects/llm_d/tests/test_profiles.pyprojects/llm_d/toolbox/cleanup_test_resources/main.py
💤 Files with no reviewable changes (2)
- projects/llm_d/orchestration/prepare_phase.py
- projects/llm_d/orchestration/phase_inputs.py
🚧 Files skipped from review as they are similar to previous changes (11)
- projects/llm_d/orchestration/manifests/kueue-localqueue.yaml
- projects/llm_d/orchestration/config.d/platform.yaml
- projects/llm_d/orchestration/presets.d/cks.yaml
- projects/llm_d/orchestration/config.yaml
- projects/llm_d/orchestration/config.d/runtime.yaml
- projects/llm_d/toolbox/cleanup_test_resources/main.py
- projects/core/dsl/utils/k8s.py
- projects/core/dsl/task.py
- projects/llm_d/tests/test_deployment_profiles.py
- projects/llm_d/tests/test_profiles.py
- projects/llm_d/orchestration/render_inference_service.py
| if args.cluster_role and not args.cluster_role_binding_name: | ||
| raise ValueError( | ||
| "cluster_role_binding_name must be provided when cluster_role is specified" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor the documented default binding name.
Line 72 sets ctx.cluster_role_binding_name to <sa_name>-binding, but Line 86 checks the still-empty args.cluster_role_binding_name. Therefore --cluster-role=view without an explicit binding name always fails despite the documented default.
Proposed fix
- if args.cluster_role and not args.cluster_role_binding_name:
+ if args.cluster_role and not ctx.cluster_role_binding_name:📝 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 args.cluster_role and not args.cluster_role_binding_name: | |
| raise ValueError( | |
| "cluster_role_binding_name must be provided when cluster_role is specified" | |
| ) | |
| if args.cluster_role and not ctx.cluster_role_binding_name: | |
| raise ValueError( | |
| "cluster_role_binding_name must be provided when cluster_role is specified" | |
| ) |
🤖 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/cluster/toolbox/create_sa_kubeconfig/main.py` around lines 86 - 89,
Update the validation in the main argument-processing flow to check the resolved
ctx.cluster_role_binding_name, established by the defaulting logic, instead of
the raw args.cluster_role_binding_name. Ensure --cluster-role without an
explicit binding name uses the documented <sa_name>-binding default while still
rejecting cases where the resolved binding name is absent.
| # Write kubeconfig to artifacts directory | ||
| kubeconfig_path = args.artifact_dir / "artifacts" / ctx.kubeconfig_file | ||
| with open(kubeconfig_path, "w") as f: | ||
| yaml.dump(kubeconfig, f, default_flow_style=False) | ||
|
|
||
| # Also write to current directory for convenience | ||
| current_dir_path = Path(ctx.kubeconfig_file) | ||
| with open(current_dir_path, "w") as f: | ||
| yaml.dump(kubeconfig, f, default_flow_style=False) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the artifact copy inside artifacts/. args.artifact_dir / "artifacts" / ctx.kubeconfig_file can escape the artifacts directory when kubeconfig_file is absolute, and nested relative paths fail unless their parent directories already exist. Use a sanitized basename or create the artifact parents separately from the user-facing output path.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 290-290: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(kubeconfig_path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 295-295: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(current_dir_path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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/cluster/toolbox/create_sa_kubeconfig/main.py` around lines 289 -
297, The kubeconfig artifact write must always remain within the artifacts
directory and support nested filenames. Update the artifact path handling near
kubeconfig_path to sanitize or reduce ctx.kubeconfig_file to a safe artifact
name, create any required artifact parent directories before writing, and keep
the separate current_dir_path output behavior unchanged.
🔴 Execution of
|
🔴 Submission of
|
…in the __test_labels__.yaml
…tom catalog is enabled
…r prefill/decode pods
Replace the stub run_kpi_analysis with a working regression engine that: - Matches current KPIs to historical baselines via configurable comparison_keys/ignored_keys on record labels - Runs per-KPI regression tests (relative threshold, higher/lower-is-better) - Produces a structured YAML report with processed/tested/results/overall - Supports plugin-provided AnalysisConfig for per-project tuning Exit codes: 0=pass, 1=error, 2=no-baseline, 3=regression-detected. Includes end-to-end test suite (15 tests). Co-authored-by: Cursor <cursoragent@cursor.com>
3989320 to
57873b3
Compare
|
/test fournos llm_d smoke |
🔴 Execution of
|
🔴 Submission of
|
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary by CodeRabbit