Skip to content

[llm-d] Start testing P/D on CKS - #136

Open
kpouget wants to merge 67 commits into
openshift-psap:mainfrom
kpouget:llmd-testing
Open

[llm-d] Start testing P/D on CKS#136
kpouget wants to merge 67 commits into
openshift-psap:mainfrom
kpouget:llmd-testing

Conversation

@kpouget

@kpouget kpouget commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features
    • Added template-based Prefill/Decode (P/D) deployment profiles with strict parameter extraction and improved P/D manifest rendering (pod/env/resource sizing and P/D metadata).
    • Added a configurable “wait” option for FOURNOS submission.
  • Bug Fixes
    • Improved preset inheritance safety when optional inheritance fields are unset.
    • More reliable artifact directory naming and updated “latest” artifact symlink handling.
    • Enhanced preflight CRD checking and improved endpoint resolution behavior when address matching isn’t available.
  • Documentation
    • Added a Prefill/Decode testing guide and expanded the orchestration changelog.
  • Tests
    • Added automated manifest generation/comparison coverage for deployment presets; updated suite expectations for the new rendering/config behavior.

@openshift-ci

openshift-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign harshith-umesh for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This 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.

Changes

Core runtime and toolbox workflows

Layer / File(s) Summary
Core execution and artifact behavior
projects/core/library/*, projects/core/dsl/*
Preset inheritance, artifact naming, post-processing results, retry logging, notification output, and command output redirection are updated.
Job and deployment toolbox workflows
projects/fournos_launcher/..., projects/kserve/toolbox/..., projects/llm_d/toolbox/...
Fournos waiting supports early return and staged polling; KServe endpoint, pod, cache, cleanup, and state-capture handling are updated.
Service-account kubeconfig workflow
projects/cluster/toolbox/create_sa_kubeconfig/main.py
Adds validation, service-account and RBAC setup, token handling, kubeconfig generation, access testing, and usage output.

llm-d deployment orchestration

Layer / File(s) Summary
Deployment profiles and scheduler configuration
projects/llm_d/orchestration/config.d/*, projects/llm_d/orchestration/manifests/*, projects/llm_d/orchestration/presets.d/*
Configuration adds P/D and name-based templates, centralized VLLM settings, explicit runtime namespace and router settings, scheduler manifests, namespace labels, Kueue resources, and simplified preset composition.
Runtime profile resolution and run specs
projects/llm_d/orchestration/runtime_config.py, projects/llm_d/orchestration/phase_inputs.py, projects/llm_d/orchestration/prepare_phase.py
Runtime configuration resolves FROM_NAME values and scheduler templates, uses explicit namespaces and simplified artifact names, and removes managed-namespace fields from phase inputs and cache preparation.
Inference service rendering and test wiring
projects/llm_d/orchestration/render_inference_service.py, projects/llm_d/orchestration/test_phase.py
Rendering supports standard and P/D deployments, merged VLLM arguments, profile annotations, model-cache handling, and Kueue metadata. Test phases consume nested KServe settings and runtime namespace labels.
Deployment references and validation
projects/llm_d/tests/*, projects/llm_d/docs/PD_TESTING.md, projects/llm_d/CHANGELOG-orchestration.md
Reference manifests, tests, documentation, and changelog entries cover standard, intelligent-routing, and P/D profiles.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: starting P/D testing on CKS.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 13

🧹 Nitpick comments (5)
projects/fournos_launcher/toolbox/submit_and_wait/main.py (2)

219-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated status-polling logic between wait_for_job_to_resolve and wait_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 win

Docstring not updated for new wait parameter.

📝 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 | 🔵 Trivial

Tracked 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 value

Drop the stale namespace_override read from activate_run_spec. runtime.namespace_override is no longer in runtime.yaml, and _get_runtime_value() returns None for 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 win

Naming doesn't match actual config schema.

This describes deployments.defaults.vllm_extra_args / profile vllm_extra_args (flat, underscored), but the actual schema uses nested vllm_extra: { args: [...] } (see deployments.pd.vllm_extra.args used via config.project.get_config(...) in render_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 (see render_inference_service.py review).

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1fa202a and b259af5.

📒 Files selected for processing (23)
  • projects/core/library/config.py
  • projects/core/library/env.py
  • projects/fournos_launcher/toolbox/submit_and_wait/main.py
  • projects/llm_d/CHANGELOG-orchestration.md
  • projects/llm_d/docs/PD_TESTING.md
  • projects/llm_d/orchestration/config.d/deployments.yaml
  • projects/llm_d/orchestration/config.d/platform.yaml
  • projects/llm_d/orchestration/config.d/project.yaml
  • projects/llm_d/orchestration/config.d/runtime.yaml
  • projects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yaml
  • projects/llm_d/orchestration/manifests/llminferenceservice.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/__router_template__.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yaml
  • projects/llm_d/orchestration/presets.d/presets.yaml
  • projects/llm_d/orchestration/render_inference_service.py
  • projects/llm_d/orchestration/runtime_config.py
  • projects/llm_d/orchestration/test_phase.py
  • projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.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/tests/test_deployment_profiles.py
  • projects/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

Comment thread projects/core/library/config.py
Comment thread projects/core/library/env.py
Comment on lines +213 to 216
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}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.

Comment thread projects/fournos_launcher/toolbox/submit_and_wait/main.py Outdated
Comment thread projects/llm_d/docs/PD_TESTING.md
Comment thread projects/llm_d/orchestration/render_inference_service.py
Comment thread projects/llm_d/orchestration/runtime_config.py Outdated
Comment thread projects/llm_d/tests/test_deployment_profiles.py
Comment thread projects/llm_d/tests/test_pd_integration.py Outdated
Comment thread projects/llm_d/tests/test_pd_integration.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 win

Update run_postprocess_after_test to return the status mapping type.

run_postprocess_after_test returns a dict on both paths, so -> None is misleading and will trip type checking. Change it to dict[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_dirname becomes the literal string "llmd__None" when no benchmark key is set.

When runtime.benchmark_key is unset, benchmark_entries = [(None, None)] and bench_key is None, so artifact_dirname=f"llmd__{bench_key}" renders as "llmd__None". This is reachable today: the new cks-pd preset (projects/llm_d/orchestration/presets.d/cks.yaml) extends cks + gpt-oss, and neither sets runtime.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 value

Remove unused constant.

PROJECT_ORCHESTRATION_DIR is 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 value

Comment 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 FileNotFoundError with 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 win

Hardcoded personal namespace (kpouget-dev) in a shared preset file.

This preset (usable by anyone applying cks/cks-pd) hardcodes a personal dev namespace name. Since runtime.namespace no longer has an auto-derivation fallback (see platform.yaml/runtime_config.py changes), any other user applying this preset unmodified will attempt to deploy into kpouget-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

📥 Commits

Reviewing files that changed from the base of the PR and between b259af5 and 09c5bde.

📒 Files selected for processing (29)
  • projects/core/library/env.py
  • projects/core/library/postprocess.py
  • projects/fournos_launcher/toolbox/submit_and_wait/main.py
  • projects/kserve/toolbox/prepare_hf_model_cache/main.py
  • projects/llm_d/CHANGELOG-orchestration.md
  • projects/llm_d/docs/PD_TESTING.md
  • projects/llm_d/orchestration/config.d/deployments.yaml
  • projects/llm_d/orchestration/config.d/platform.yaml
  • projects/llm_d/orchestration/config.d/project.yaml
  • projects/llm_d/orchestration/config.d/runtime.yaml
  • projects/llm_d/orchestration/config.yaml
  • projects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yaml
  • projects/llm_d/orchestration/manifests/llminferenceservice.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/__router_template__.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yaml
  • projects/llm_d/orchestration/phase_inputs.py
  • projects/llm_d/orchestration/prepare_phase.py
  • projects/llm_d/orchestration/presets.d/cks.yaml
  • projects/llm_d/orchestration/presets.d/cpt.yaml
  • projects/llm_d/orchestration/presets.d/presets.yaml
  • projects/llm_d/orchestration/render_inference_service.py
  • projects/llm_d/orchestration/runtime_config.py
  • projects/llm_d/orchestration/test_phase.py
  • projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.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/tests/test_deployment_profiles.py
  • projects/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

Comment thread projects/llm_d/orchestration/config.d/platform.yaml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
projects/llm_d/orchestration/test_phase.py (1)

34-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the enabled Kueue path.

The existing profile tests force runtime.kserve.dry_run=True in projects/llm_d/tests/test_deployment_profiles.py:43-60, so ensure_kueue_local_queue() is never exercised. Add a focused test that mocks oc and verifies the rendered name, namespace, and clusterQueue fields.

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between 09c5bde and 76da508.

📒 Files selected for processing (10)
  • projects/core/dsl/task.py
  • projects/core/dsl/utils/k8s.py
  • projects/fournos_launcher/toolbox/submit_and_wait/main.py
  • projects/kserve/toolbox/deploy_llmisvc/main.py
  • projects/llm_d/orchestration/config.d/runtime.yaml
  • projects/llm_d/orchestration/config.yaml
  • projects/llm_d/orchestration/manifests/kueue-localqueue.yaml
  • projects/llm_d/orchestration/presets.d/cks.yaml
  • projects/llm_d/orchestration/render_inference_service.py
  • projects/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

Comment thread projects/kserve/toolbox/deploy_llmisvc/main.py
Comment thread projects/kserve/toolbox/deploy_llmisvc/main.py
@kpouget

kpouget commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/test fournos llm_d cks-pd
/cluster cks-warldorf
/pipeline forge-test-only

@psap-forge-bot

Copy link
Copy Markdown
🔴 Submission of llm_d cks-pd failed after 5 seconds 🔴

Error: FournosJobFailureError: FOURNOS Job 'forge-llm-d-20260722-074012' failed: Job failed in its early stages: Cluster 'cks-warldorf' not found

/test fournos llm_d cks-pd
/cluster cks-warldorf
/pipeline forge-test-only

@kpouget

kpouget commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

@psap-forge-bot

Copy link
Copy Markdown

🔴 Execution of llm_d cks-pd 🔴

Execution Engine Configuration

forge:
  args:
  - cks-pd
  configOverrides: {}
  project: llm_d

Artifact Links

Test Logs

00 Preflight 1 second

🔄 02 Export-Artifacts

@psap-forge-bot

Copy link
Copy Markdown
🔴 Submission of llm_d cks-pd failed after 1 minute, 29 seconds 🔴

Error: FournosJobFailureError: FOURNOS Job 'forge-llm-d-20260722-074132' failed: Tasks Completed: 2 (Failed: 1, Cancelled 0), Skipped: 1

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

@kpouget

kpouget commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 76da508 and e92ea5f.

📒 Files selected for processing (35)
  • projects/core/dsl/task.py
  • projects/core/dsl/utils/k8s.py
  • projects/core/library/config.py
  • projects/core/library/env.py
  • projects/core/library/postprocess.py
  • projects/fournos_launcher/toolbox/submit_and_wait/main.py
  • projects/kserve/toolbox/deploy_llmisvc/main.py
  • projects/kserve/toolbox/prepare_hf_model_cache/main.py
  • projects/llm_d/CHANGELOG-orchestration.md
  • projects/llm_d/docs/PD_TESTING.md
  • projects/llm_d/orchestration/config.d/deployments.yaml
  • projects/llm_d/orchestration/config.d/platform.yaml
  • projects/llm_d/orchestration/config.d/project.yaml
  • projects/llm_d/orchestration/config.d/runtime.yaml
  • projects/llm_d/orchestration/config.yaml
  • projects/llm_d/orchestration/manifests/deployments/precise-prefix-cache.yaml
  • projects/llm_d/orchestration/manifests/kueue-localqueue.yaml
  • projects/llm_d/orchestration/manifests/llminferenceservice.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/__router_template__.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/approximate-prefix-cache.yaml
  • projects/llm_d/orchestration/manifests/scheduler_config/precise-prefix-cache.yaml
  • projects/llm_d/orchestration/phase_inputs.py
  • projects/llm_d/orchestration/prepare_phase.py
  • projects/llm_d/orchestration/presets.d/cks.yaml
  • projects/llm_d/orchestration/presets.d/cpt.yaml
  • projects/llm_d/orchestration/presets.d/presets.yaml
  • projects/llm_d/orchestration/render_inference_service.py
  • projects/llm_d/orchestration/runtime_config.py
  • projects/llm_d/orchestration/test_phase.py
  • projects/llm_d/tests/reference_deployments/cpt-reference-flavors/deployment-intelligentrouting-tp4-x4/llmisvc.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/tests/test_deployment_profiles.py
  • projects/llm_d/tests/test_profiles.py
  • projects/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

Comment thread projects/llm_d/orchestration/presets.d/cks.yaml
Comment thread projects/llm_d/tests/test_deployment_profiles.py Outdated
Comment thread projects/llm_d/tests/test_deployment_profiles.py
@psap-forge-bot

Copy link
Copy Markdown

🔴 Execution of llm_d cks-pd 🔴

Execution Engine Configuration

forge:
  args:
  - cks-pd
  configOverrides: {}
  project: llm_d

Artifact Links

Test Logs

00 Preflight 3 seconds

🔄 02 Export-Artifacts

@psap-forge-bot

Copy link
Copy Markdown
🔴 Submission of llm_d cks-pd failed after 8 minutes, 55 seconds 🔴

Error: FournosJobFailureError: FOURNOS Job 'forge-llm-d-20260722-100337' failed: Tasks Completed: 2 (Failed: 1, Cancelled 0), Skipped: 1

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

@kpouget
kpouget force-pushed the llmd-testing branch 2 times, most recently from a91c7d6 to 93a156e Compare July 22, 2026 10:25
@kpouget

kpouget commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

@psap-forge-bot

Copy link
Copy Markdown

🔴 Execution of llm_d cks-pd 🔴

Execution Engine Configuration

forge:
  args:
  - cks-pd
  configOverrides: {}
  project: llm_d

Artifact Links

Test Logs

00 Preflight 3 seconds

01 Test 18 minutes, 17 seconds

🔄 02 Export-Artifacts

Post-processing Status

@psap-forge-bot

Copy link
Copy Markdown
🔴 Submission of llm_d cks-pd failed after 20 minutes, 7 seconds 🔴

Error: FournosJobFailureError: FOURNOS Job 'forge-llm-d-20260722-102621' failed: Tasks Completed: 3 (Failed: 1, Cancelled 0), Skipped: 0

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

@kpouget

kpouget commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 win

Test/post-processing status is skipped when there are no step logs.

_build_test_status_section(status) is only invoked inside if step_log_links:, but step_log_links can be empty even when status["test_phase"] is populated (e.g. an early failure before any step directory has a run.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 win

Document the new wait parameter.

The Args: docstring section wasn't updated for the new wait: bool = True parameter.

♻️ 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

📥 Commits

Reviewing files that changed from the base of the PR and between e92ea5f and 59563f4.

📒 Files selected for processing (22)
  • projects/cluster/toolbox/create_sa_kubeconfig/main.py
  • projects/core/dsl/runtime.py
  • projects/core/dsl/task.py
  • projects/core/dsl/utils/k8s.py
  • projects/core/library/export.py
  • projects/fournos_launcher/toolbox/submit_and_wait/main.py
  • projects/kserve/toolbox/capture_llmisvc_state/main.py
  • projects/kserve/toolbox/deploy_llmisvc/main.py
  • projects/kserve/toolbox/prepare_hf_model_cache/main.py
  • projects/llm_d/orchestration/config.d/platform.yaml
  • projects/llm_d/orchestration/config.d/runtime.yaml
  • projects/llm_d/orchestration/config.yaml
  • projects/llm_d/orchestration/manifests/kueue-localqueue.yaml
  • projects/llm_d/orchestration/phase_inputs.py
  • projects/llm_d/orchestration/preflight_phase.py
  • projects/llm_d/orchestration/prepare_phase.py
  • projects/llm_d/orchestration/presets.d/cks.yaml
  • projects/llm_d/orchestration/render_inference_service.py
  • projects/llm_d/orchestration/test_phase.py
  • projects/llm_d/tests/test_deployment_profiles.py
  • projects/llm_d/tests/test_profiles.py
  • projects/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

Comment on lines +86 to +89
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"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.

Suggested change
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.

Comment thread projects/cluster/toolbox/create_sa_kubeconfig/main.py Outdated
Comment on lines +289 to +297
# 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.

Comment thread projects/fournos_launcher/toolbox/submit_and_wait/main.py
Comment thread projects/kserve/toolbox/prepare_hf_model_cache/main.py
Comment thread projects/llm_d/orchestration/preflight_phase.py
Comment thread projects/llm_d/orchestration/test_phase.py
@psap-forge-bot

Copy link
Copy Markdown

🔴 Execution of llm_d cks-pd 🔴

Execution Engine Configuration

forge:
  args:
  - cks-pd
  configOverrides: {}
  project: llm_d

Artifact Links

Test Logs

00 Preflight 1 second

01 Test 18 minutes, 22 seconds

🔄 02 Export-Artifacts

Post-processing Status

@psap-forge-bot

Copy link
Copy Markdown
🔴 Submission of llm_d cks-pd failed after 20 minutes, 17 seconds 🔴

Error: FournosJobFailureError: FOURNOS Job 'forge-llm-d-20260722-111638' failed: Tasks Completed: 3 (Failed: 1, Cancelled 0), Skipped: 0

/test fournos llm_d cks-pd
/cluster cks-waldorf
/pipeline forge-test-only

kpouget and others added 17 commits July 31, 2026 17:34
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>
@kpouget
kpouget force-pushed the llmd-testing branch 2 times, most recently from 3989320 to 57873b3 Compare July 31, 2026 15:53
@kpouget

kpouget commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

/test fournos llm_d smoke
/pipeline forge-test-only
/cluster forge-smoke-testing

@psap-forge-bot

Copy link
Copy Markdown

🔴 Execution of llm_d smoke 🔴

Execution Engine Configuration

forge:
  args:
  - smoke
  configOverrides: {}
  project: llm_d

Artifact Links

Test Logs

00 Preflight 2 seconds

01 Test 8 seconds

🔄 02 Export-Artifacts

Post-processing Status

  • artifacts_to_kpis: success
  • ⏭️ kpis_to_csv: disabled

    kpi.kpis_to_csv disabled

  • ⏭️ artifacts_to_ai_data: disabled

    kpi.artifacts_to_ai_data disabled

  • ⏭️ s3_import: disabled

    s3_import disabled

  • analyse_kpis: failed
  • ⏭️ s3_export: disabled

    s3_export disabled

@psap-forge-bot

Copy link
Copy Markdown
🔴 Submission of llm_d smoke failed after 6 minutes, 32 seconds 🔴

Error: FournosJobFailureError: FOURNOS Job 'forge-llm-d-20260731-155729' failed: Tasks Completed: 3 (Failed: 1, Cancelled 0), Skipped: 0

/test fournos llm_d smoke
/pipeline forge-test-only
/cluster forge-smoke-testing

@openshift-ci

openshift-ci Bot commented Jul 31, 2026

Copy link
Copy Markdown

@kpouget: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/fournos 51c9510 link true /test fournos

Full PR test history. Your PR dashboard.

Details

Instructions 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants