feat(pipelines): add evalhub pipelines and small tweaks to existing lora pipelines#81
Open
MStokluska wants to merge 2 commits into
Open
feat(pipelines): add evalhub pipelines and small tweaks to existing lora pipelines#81MStokluska wants to merge 2 commits into
MStokluska wants to merge 2 commits into
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 |
📝 WalkthroughWalkthroughAdds a new evalhub_kserve KFP component that serves a model with KServe, submits evaluations to Eval Hub, polls results, emits KFP metrics/artifacts, and always cleans up KServe resources. Introduces six pipeline families (LoRA/OSFT/SFT each with full and minimal variants) that wire dataset download, training, KServe-based evaluation, and optional model registry steps. Removes three MLflow parameters from the existing LoRA component and pipeline signatures. Adds metadata, tests, READMEs, and OWNERS entries. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Key Security and Code Quality IssuesCWE-345: Insufficient Verification of Data Authenticity
CWE-613: Insufficient Session Expiration
CWE-400: Uncontrolled Resource Consumption
CWE-295 / CWE-200: Improper Certificate Validation / Sensitive Data Exposure
CWE-248: Uncaught Exception / File I/O Robustness
CWE-706: Use of Incorrectly-Resolved Name
CWE-427 (Supply-chain) / Image pinning
CWE-522 / CWE-259: Insufficiently Protected Credentials
Pipeline / Operational Risks
Migration note for LoRA MLflow removal
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
pipelines/training/finetuning/lora_minimal_evalhub/README.md (1)
1-89:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winREADME is out of sync and must be regenerated.
Pipeline validation detected 142 lines of diff. Run the README generator with
--fixto synchronize.🤖 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 `@pipelines/training/finetuning/lora_minimal_evalhub/README.md` around lines 1 - 89, The README for lora_minimal_evalhub_pipeline is out-of-sync (142 lines differ); regenerate the file using the project README generator to sync docs—run the README generator tool/command used in this repo with the --fix flag (the same process that produced README for lora_minimal_evalhub) so the generated content (overview, inputs table, metadata, dependencies, etc.) is updated to match the current pipeline spec and remove the detected diff.pipelines/training/finetuning/lora_evalhub/README.md (1)
1-127:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winREADME is out of sync and must be regenerated.
Pipeline validation detected 218 lines of diff. Run the README generator with
--fixto synchronize.🤖 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 `@pipelines/training/finetuning/lora_evalhub/README.md` around lines 1 - 127, The README for the lora_evalhub pipeline is out of sync (pipeline validation found 218 lines of diff); run the project's README generator to regenerate and synchronize it by invoking the README generator for the pipelines/training/finetuning/lora_evalhub asset with the --fix flag (ensure you target the lora_evalhub/ README generation task or command used by the repo CI so files like README.md are overwritten with the canonical, generated content), then commit the updated README.md so the pipeline validation passes.pipelines/training/finetuning/sft_minimal_evalhub/README.md (1)
1-86:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winREADME must be regenerated before merge.
CI detected 123 lines of drift. Run the repository's README synchronization tool with
--fixto update this file from the pipeline source.🤖 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 `@pipelines/training/finetuning/sft_minimal_evalhub/README.md` around lines 1 - 86, The README for the sft_minimal_evalhub_pipeline is out of sync with the pipeline source; run the repository's README synchronization tool to regenerate this file (e.g., invoke the README sync command used by CI with the --fix flag) so the README content matches the pipeline source and removes the reported 123-line drift; update the sft_minimal_evalhub/README.md (the file under review) and commit the regenerated file before merging.pipelines/training/finetuning/sft_evalhub/README.md (1)
1-102:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winREADME must be regenerated before merge.
CI detected 154 lines of drift. Run the repository's README synchronization tool with
--fixto update this file from the pipeline source.🤖 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 `@pipelines/training/finetuning/sft_evalhub/README.md` around lines 1 - 102, The README (sft_evalhub_pipeline README.md) is out of sync with the pipeline source (CI reports 154 lines drift); regenerate it by running the repository README synchronization tool with the --fix flag (e.g., the repo's README sync command that updates pipeline docs), update the generated README.md for the sft_evalhub pipeline, verify the diffs are resolved (no drift), and commit the regenerated README so CI passes.pipelines/training/finetuning/sft_minimal_evalhub/pipeline.py (1)
1-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix formatting to unblock CI.
Lines 1-244 fail
ruff format --checkin CI. This blocks merge; format the file and rerun lint.🤖 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 `@pipelines/training/finetuning/sft_minimal_evalhub/pipeline.py` around lines 1 - 244, The file fails ruff formatting checks; run ruff format on this module (or run your project formatter that CI uses) and commit the resulting changes to fix whitespace, import ordering, docstring/line-wrapping and trailing-comma styles around the top-level constants (PVC_SIZE, PVC_STORAGE_CLASS, PIPELINE_NAME) and the pipeline function definition sft_pipeline_evalhub_easy (including its decorator and long parameter list), then re-run ruff format --check (and CI) to ensure the formatting error is resolved.pipelines/training/finetuning/osft_minimal_evalhub/README.md (1)
1-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winREADME sync check is failing for this file; regenerate before merge.
CI reports this README out of sync (139-line diff). Run the project’s README sync command with
--fixand commit the updated generated file.🤖 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 `@pipelines/training/finetuning/osft_minimal_evalhub/README.md` around lines 1 - 88, The README for the osft_minimal_evalhub_pipeline is out of sync (139-line diff); run the project's README sync command with the --fix flag to regenerate the pipeline README (the README.md for osft_minimal_evalhub), then commit the updated generated file so the CI diff disappears and re-run CI to confirm the README is in sync.pipelines/training/finetuning/osft_evalhub/README.md (1)
1-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerate this README; CI is currently failing on sync validation.
The pipeline log reports this file is out of sync (
README Validation, 173-line diff). Re-run the repo README validation/generation command with--fixand commit the generated result.🤖 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 `@pipelines/training/finetuning/osft_evalhub/README.md` around lines 1 - 105, CI reports pipelines/training/finetuning/osft_evalhub/README.md is out of sync; regenerate the README using the repository's README validation/generation tool with the --fix flag (the same command CI uses for "README Validation") to update osft_evalhub README, then commit the regenerated file so the 173-line diff is resolved and the pipeline README is in sync.components/evaluation/evalhub_kserve/README.md (1)
1-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winREADME sync check is failing for this file.
The CI “README files are up-to-date” job is failing; regenerate/sync this README with the repo’s README tooling before merge.
🤖 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 `@components/evaluation/evalhub_kserve/README.md` around lines 1 - 115, The README for the evalhub_kserve component (components/evaluation/evalhub_kserve/README.md) is out of sync with the repo's generated docs; regenerate or sync it using the repository's README tooling (e.g., the README generator or make target used by CI) so the file for the evalhub_kserve component (README.md under evalhub_kserve) matches the canonical output, then commit the regenerated README so the “README files are up-to-date” CI job passes.components/evaluation/evalhub_kserve/component.py (1)
1-578:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFormatting check is currently blocking CI for this file.
ruff format --checkfailed; this file needs formatting before merge.🤖 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 `@components/evaluation/evalhub_kserve/component.py` around lines 1 - 578, The file fails ruff formatting; run the project's formatter (ruff format) on the component file to apply the required style changes and re-run ruff format --check; specifically reformat the function evalhub_evaluator_kserve and its helper functions (_create_serving_runtime, _create_inference_service, _wait_for_isvc_ready, _get_isvc_url, etc.), commit the reformatted file, and ensure CI passes with no remaining ruff format errors.
🧹 Nitpick comments (2)
pipelines/training/finetuning/osft_evalhub/pipeline.py (1)
90-96: ⚡ Quick winUse None as default for benchmarks parameter to avoid mutable default anti-pattern.
Line 90 defines a list default that persists across invocations. While the parameter is not mutated in this function, this pattern is a Python code smell and creates fragile code—future maintainers could inadvertently add mutations that cause state leakage between calls. Use the None sentinel pattern instead.
Safer default initialization
- phase_03_eval_opt_benchmarks: list = [ - {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, - ], + phase_03_eval_opt_benchmarks: list | None = None, @@ ): + if phase_03_eval_opt_benchmarks is None: + phase_03_eval_opt_benchmarks = [ + {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, + ]🤖 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 `@pipelines/training/finetuning/osft_evalhub/pipeline.py` around lines 90 - 96, The default for phase_03_eval_opt_benchmarks is a mutable list which can lead to state leakage; change the parameter/variable to use None as the default (phase_03_eval_opt_benchmarks: Optional[list] = None) and initialize the list inside the function or initializer where it's used (e.g., if phase_03_eval_opt_benchmarks is None: phase_03_eval_opt_benchmarks = [ {"id":"leaderboard_ifeval","provider_id":"lm_evaluation_harness"}, ... ]), ensuring the unique name phase_03_eval_opt_benchmarks is updated and any callers still pass an explicit list when needed.components/evaluation/evalhub_kserve/component.py (1)
32-32: ⚡ Quick winReplace mutable default argument with None.
benchmarks: list = []uses a shared list object across function calls. Change tobenchmarks: list | None = Noneand normalize at the use site.Suggested fix
- benchmarks: list = [], + benchmarks: list | None = None,🤖 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 `@components/evaluation/evalhub_kserve/component.py` at line 32, The parameter benchmarks currently uses a mutable default (benchmarks: list = []); change its annotation to allow None (benchmarks: list | None = None) and inside the function or initializer (wherever benchmarks is used in component.py, e.g., in the constructor or the function that declares benchmarks) normalize it by replacing None with an empty list (e.g., if benchmarks is None: benchmarks = []) before any mutations or iterations; update any type hints or downstream usages accordingly.
🤖 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 `@components/evaluation/evalhub_kserve/component.py`:
- Around line 342-347: The code currently assumes model_path maps to a mounted
filesystem by assigning to final_model_path and then raising if none; instead
validate whether model_path is a local filesystem path before treating it as a
PVC path (use os.path.exists / Path(...).exists on model_path) and if it is not
local either (A) explicitly reject non-local IDs by raising a clear ValueError
mentioning that model_path must be a mounted PVC path (and referencing the
HuggingFace ID case), or (B) implement a non-PVC branch that handles remote
model IDs (e.g., detect HF-style IDs and download/resolve them or load via the
HF/transformers API) so final_model_path is only set for actual local paths and
the ValueError raised from the current ValueError(...) only triggers when no
valid local or resolved remote artifact exists; update the logic around
final_model_path and model_path to reflect this validation/resolution.
- Around line 308-312: The _cleanup_kserve function currently calls _k8s_api to
DELETE the InferenceService and ServingRuntime but ignores the responses and
always logs success; update _cleanup_kserve to inspect the return values/status
codes from _k8s_api for both DELETE calls, log an error (with details) and
either retry/fail fast or raise/return a non-success indicator when a delete
fails, and only log "Cleanup complete" if both deletes returned success;
reference the _cleanup_kserve function and the two _k8s_api DELETE calls for
locating the change.
- Around line 446-447: The requests.post calls that send the ServiceAccount
token (e.g., the call that assigns resp from requests.post(submit_url,
json=eval_config, headers=headers, timeout=30, verify=False)) disable TLS
verification; change them to perform proper certificate validation (remove
verify=False or set verify to a CA bundle path) and, if needed, allow
configuring a trusted CA via configuration/environment; update all similar calls
in this component (the other requests.post calls around the same submit flow) to
use verified TLS and ensure headers (bearer token) are only sent over verified
HTTPS connections.
- Around line 154-155: The current check if model_path.startswith(mount_path)
can falsely match prefixes (e.g., "/mnt/models2" matches "/mnt/models"); change
the logic in the block that looks up pvc_volumes by vol_name to normalize paths
(os.path.normpath) and then either compare the mount_path as an exact path
component prefix using os.path.commonpath([normalized_mount_path,
normalized_model_path]) == normalized_mount_path or require startswith with a
separator (mount_path + os.sep) while handling root mounts; update the code that
references pvc_volumes, vol_name, model_path, and mount_path to use this
normalized/commonpath approach so the correct PVC mount is selected.
- Around line 193-199: The component unconditionally adds "--trust-remote-code",
enabling arbitrary code execution; add a new parameter trust_remote_code: bool =
False to evalhub_evaluator_kserve, propagate that parameter into the call to
_create_serving_runtime, and inside _create_serving_runtime only append
"--trust-remote-code" to the container args list when trust_remote_code is True;
finally update the call site that constructs the serving runtime (the existing
call to _create_serving_runtime around where the runtime is built) to pass the
new trust_remote_code argument.
In `@components/evaluation/evalhub_kserve/metadata.yaml`:
- Line 1: Add a YAML document start marker to the top of the metadata YAML so
the linter passes; update the metadata.yaml that declares "name: evalhub_kserve"
by inserting the YAML document start `---` before the existing content (ensure
it's the very first line of the file).
In `@components/evaluation/evalhub_kserve/README.md`:
- Around line 76-78: The README's "kubernetes-credentials" secret requirement
conflicts with the component's actual auth method: the component reads
in-cluster ServiceAccount files (e.g., /var/run/secrets/...) rather than a
secret named kubernetes-credentials; update the README in
components/evaluation/evalhub_kserve/README.md to reflect the real contract by
removing or clarifying the kubernetes-credentials secret requirement and
documenting that the component uses in-cluster ServiceAccount token and CA from
/var/run/secrets/... for K8s API access (or, alternatively, document how to
supply credentials via a kubernetes-credentials secret if you change the
component to use that instead).
In `@pipelines/training/finetuning/lora_evalhub/metadata.yaml`:
- Line 29: The links.documentation entry in metadata.yaml is pointing to the
Trainer repo instead of the pipeline docs; update the documentation URL under
the metadata key (links.documentation) in
pipelines/training/finetuning/lora_evalhub/metadata.yaml to the correct pipeline
documentation location by replacing "https://github.com/kubeflow/trainer" with
the pipeline's actual docs URL (ensure the new URL is the canonical pipeline
documentation page).
In `@pipelines/training/finetuning/lora_minimal_evalhub/metadata.yaml`:
- Line 1: The YAML file defining the pipeline (metadata.yaml with the document
name "lora_minimal_evalhub_pipeline") is missing the YAML document start marker;
add the standard '---' marker as the first line of metadata.yaml so the file
begins with a YAML document start before the existing "name:
lora_minimal_evalhub_pipeline" entry to satisfy yaml-lint and avoid lint
failures.
In `@pipelines/training/finetuning/lora_minimal_evalhub/pipeline.py`:
- Around line 51-53: The pipeline accepts unvalidated URIs in the parameters
phase_01_dataset_man_data_uri and phase_03_eval_opt_evalhub_url which are later
used (around the current uses at lines ~113 and ~186); add a validation step
before Stage 1/3 that parses and validates these inputs (e.g., with urllib.parse
or a URL validator), enforce an allowlist of acceptable schemes (http/https),
disallow file:, file system paths, localhost, and private IP ranges, normalize
the path to avoid traversal, and raise/return a clear error if validation fails
so downstream stages never receive untrusted/internal targets.
In `@pipelines/training/finetuning/osft_evalhub/README.md`:
- Around line 25-33: The RBAC role evalhub-kserve-role currently grants broad
secret access (verbs=create,delete,get,list,patch on resource=secrets) which is
over-privileged; update the role to remove unrestricted secrets access (or move
secret rules into a separate narrowly-scoped role) and instead grant only the
minimal required verbs on secrets or use resourceNames to limit to specific
secret names, then bind evalhub-kserve-binding to the least-privileged role
appropriate for the pipeline ServiceAccount; ensure no rule leaves
create/delete/list/patch on all secrets unless absolutely necessary.
In `@pipelines/training/finetuning/osft_minimal_evalhub/README.md`:
- Around line 26-33: Remove broad "secrets" permission from the existing
evalhub-kserve-role and instead create a separate, minimal read-only secrets
role (e.g., evalhub-secrets-role) that uses verbs=get,list and is scoped via
resourceNames to only the specific secret object names needed; keep KServe
resources in evalhub-kserve-role, then create a second RoleBinding (e.g.,
evalhub-secrets-binding) that binds the new evalhub-secrets-role to the same
service account (<namespace>:<pipeline-sa>), ensuring evalhub-kserve-binding no
longer grants secrets access.
In `@pipelines/training/finetuning/sft_evalhub/pipeline.py`:
- Around line 22-24: PVC_STORAGE_CLASS is hardcoded which prevents overrides;
make it a pipeline parameter with default "nfs-csi" (e.g., add a function
parameter like pvc_storage_class: str = "nfs-csi" to the pipeline function) and
replace uses of the constant PVC_STORAGE_CLASS with that parameter; keep
PVC_SIZE and PVC_ACCESS_MODES as defaults or convert them similarly if desired,
and apply the runtime value when configuring the PVC (use dsl.PipelineConfig
inside the pipeline or call task.set_volume / the KFP PVC configuration API to
set the storageClassName to the pvc_storage_class parameter so clusters without
"nfs-csi" can pass an alternate class).
- Line 48: The pipeline accepts arbitrary phase_01_dataset_man_data_uri and
processes it in dataset_download (uses parse_uri, S3 handling, and HTTP fetch),
enabling path traversal and SSRF; fix by enforcing a strict whitelist and
containment checks: validate scheme against allowed set (e.g., only file://
within pvc_mount_path, approved s3:// buckets/endpoints, and explicit http(s)
hosts), verify parse_uri hostnames are not local/private IPs (block 127.0.0.0/8,
10.0.0.0/8, 169.254.0.0/16, 192.168.0.0/16, ::1, fc00::/7), require S3
bucket/endpoint allowlist or exact endpoint match before using Kubernetes
credentials, and for file:/// or bare paths enforce that resolved paths are
under the intended pvc_mount_path (reject any path escaping via .. or absolute
paths outside mount); apply these checks early in dataset_download (using
parse_uri output) and fail fast with clear errors if validation fails.
In `@pipelines/training/finetuning/sft_minimal_evalhub/metadata.yaml`:
- Line 1: Add the YAML document start marker to the metadata file so YAML
linters accept it: update metadata.yaml (which currently contains the key "name:
sft_minimal_evalhub_pipeline") by prepending the document start marker '---' as
the first line so the file begins with a proper YAML document header.
---
Outside diff comments:
In `@components/evaluation/evalhub_kserve/component.py`:
- Around line 1-578: The file fails ruff formatting; run the project's formatter
(ruff format) on the component file to apply the required style changes and
re-run ruff format --check; specifically reformat the function
evalhub_evaluator_kserve and its helper functions (_create_serving_runtime,
_create_inference_service, _wait_for_isvc_ready, _get_isvc_url, etc.), commit
the reformatted file, and ensure CI passes with no remaining ruff format errors.
In `@components/evaluation/evalhub_kserve/README.md`:
- Around line 1-115: The README for the evalhub_kserve component
(components/evaluation/evalhub_kserve/README.md) is out of sync with the repo's
generated docs; regenerate or sync it using the repository's README tooling
(e.g., the README generator or make target used by CI) so the file for the
evalhub_kserve component (README.md under evalhub_kserve) matches the canonical
output, then commit the regenerated README so the “README files are up-to-date”
CI job passes.
In `@pipelines/training/finetuning/lora_evalhub/README.md`:
- Around line 1-127: The README for the lora_evalhub pipeline is out of sync
(pipeline validation found 218 lines of diff); run the project's README
generator to regenerate and synchronize it by invoking the README generator for
the pipelines/training/finetuning/lora_evalhub asset with the --fix flag (ensure
you target the lora_evalhub/ README generation task or command used by the repo
CI so files like README.md are overwritten with the canonical, generated
content), then commit the updated README.md so the pipeline validation passes.
In `@pipelines/training/finetuning/lora_minimal_evalhub/README.md`:
- Around line 1-89: The README for lora_minimal_evalhub_pipeline is out-of-sync
(142 lines differ); regenerate the file using the project README generator to
sync docs—run the README generator tool/command used in this repo with the --fix
flag (the same process that produced README for lora_minimal_evalhub) so the
generated content (overview, inputs table, metadata, dependencies, etc.) is
updated to match the current pipeline spec and remove the detected diff.
In `@pipelines/training/finetuning/osft_evalhub/README.md`:
- Around line 1-105: CI reports
pipelines/training/finetuning/osft_evalhub/README.md is out of sync; regenerate
the README using the repository's README validation/generation tool with the
--fix flag (the same command CI uses for "README Validation") to update
osft_evalhub README, then commit the regenerated file so the 173-line diff is
resolved and the pipeline README is in sync.
In `@pipelines/training/finetuning/osft_minimal_evalhub/README.md`:
- Around line 1-88: The README for the osft_minimal_evalhub_pipeline is out of
sync (139-line diff); run the project's README sync command with the --fix flag
to regenerate the pipeline README (the README.md for osft_minimal_evalhub), then
commit the updated generated file so the CI diff disappears and re-run CI to
confirm the README is in sync.
In `@pipelines/training/finetuning/sft_evalhub/README.md`:
- Around line 1-102: The README (sft_evalhub_pipeline README.md) is out of sync
with the pipeline source (CI reports 154 lines drift); regenerate it by running
the repository README synchronization tool with the --fix flag (e.g., the repo's
README sync command that updates pipeline docs), update the generated README.md
for the sft_evalhub pipeline, verify the diffs are resolved (no drift), and
commit the regenerated README so CI passes.
In `@pipelines/training/finetuning/sft_minimal_evalhub/pipeline.py`:
- Around line 1-244: The file fails ruff formatting checks; run ruff format on
this module (or run your project formatter that CI uses) and commit the
resulting changes to fix whitespace, import ordering, docstring/line-wrapping
and trailing-comma styles around the top-level constants (PVC_SIZE,
PVC_STORAGE_CLASS, PIPELINE_NAME) and the pipeline function definition
sft_pipeline_evalhub_easy (including its decorator and long parameter list),
then re-run ruff format --check (and CI) to ensure the formatting error is
resolved.
In `@pipelines/training/finetuning/sft_minimal_evalhub/README.md`:
- Around line 1-86: The README for the sft_minimal_evalhub_pipeline is out of
sync with the pipeline source; run the repository's README synchronization tool
to regenerate this file (e.g., invoke the README sync command used by CI with
the --fix flag) so the README content matches the pipeline source and removes
the reported 123-line drift; update the sft_minimal_evalhub/README.md (the file
under review) and commit the regenerated file before merging.
---
Nitpick comments:
In `@components/evaluation/evalhub_kserve/component.py`:
- Line 32: The parameter benchmarks currently uses a mutable default
(benchmarks: list = []); change its annotation to allow None (benchmarks: list |
None = None) and inside the function or initializer (wherever benchmarks is used
in component.py, e.g., in the constructor or the function that declares
benchmarks) normalize it by replacing None with an empty list (e.g., if
benchmarks is None: benchmarks = []) before any mutations or iterations; update
any type hints or downstream usages accordingly.
In `@pipelines/training/finetuning/osft_evalhub/pipeline.py`:
- Around line 90-96: The default for phase_03_eval_opt_benchmarks is a mutable
list which can lead to state leakage; change the parameter/variable to use None
as the default (phase_03_eval_opt_benchmarks: Optional[list] = None) and
initialize the list inside the function or initializer where it's used (e.g., if
phase_03_eval_opt_benchmarks is None: phase_03_eval_opt_benchmarks = [
{"id":"leaderboard_ifeval","provider_id":"lm_evaluation_harness"}, ... ]),
ensuring the unique name phase_03_eval_opt_benchmarks is updated and any callers
still pass an explicit list when needed.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 014c90af-ba36-4cdf-a342-f9fcc443402f
📒 Files selected for processing (52)
components/evaluation/evalhub_kserve/README.mdcomponents/evaluation/evalhub_kserve/__init__.pycomponents/evaluation/evalhub_kserve/component.pycomponents/evaluation/evalhub_kserve/metadata.yamlcomponents/evaluation/evalhub_kserve/tests/__init__.pycomponents/evaluation/evalhub_kserve/tests/test_component_unit.pycomponents/training/finetuning/lora/README.mdcomponents/training/finetuning/lora/component.pypipelines/training/finetuning/lora/README.mdpipelines/training/finetuning/lora/pipeline.pypipelines/training/finetuning/lora_evalhub/OWNERSpipelines/training/finetuning/lora_evalhub/README.mdpipelines/training/finetuning/lora_evalhub/__init__.pypipelines/training/finetuning/lora_evalhub/metadata.yamlpipelines/training/finetuning/lora_evalhub/pipeline.pypipelines/training/finetuning/lora_evalhub/tests/__init__.pypipelines/training/finetuning/lora_evalhub/tests/test_pipeline.pypipelines/training/finetuning/lora_minimal_evalhub/OWNERSpipelines/training/finetuning/lora_minimal_evalhub/README.mdpipelines/training/finetuning/lora_minimal_evalhub/__init__.pypipelines/training/finetuning/lora_minimal_evalhub/metadata.yamlpipelines/training/finetuning/lora_minimal_evalhub/pipeline.pypipelines/training/finetuning/lora_minimal_evalhub/tests/__init__.pypipelines/training/finetuning/lora_minimal_evalhub/tests/test_pipeline.pypipelines/training/finetuning/osft_evalhub/OWNERSpipelines/training/finetuning/osft_evalhub/README.mdpipelines/training/finetuning/osft_evalhub/__init__.pypipelines/training/finetuning/osft_evalhub/metadata.yamlpipelines/training/finetuning/osft_evalhub/pipeline.pypipelines/training/finetuning/osft_evalhub/tests/__init__.pypipelines/training/finetuning/osft_evalhub/tests/test_pipeline.pypipelines/training/finetuning/osft_minimal_evalhub/OWNERSpipelines/training/finetuning/osft_minimal_evalhub/README.mdpipelines/training/finetuning/osft_minimal_evalhub/__init__.pypipelines/training/finetuning/osft_minimal_evalhub/metadata.yamlpipelines/training/finetuning/osft_minimal_evalhub/pipeline.pypipelines/training/finetuning/osft_minimal_evalhub/tests/__init__.pypipelines/training/finetuning/osft_minimal_evalhub/tests/test_pipeline.pypipelines/training/finetuning/sft_evalhub/OWNERSpipelines/training/finetuning/sft_evalhub/README.mdpipelines/training/finetuning/sft_evalhub/__init__.pypipelines/training/finetuning/sft_evalhub/metadata.yamlpipelines/training/finetuning/sft_evalhub/pipeline.pypipelines/training/finetuning/sft_evalhub/tests/__init__.pypipelines/training/finetuning/sft_evalhub/tests/test_pipeline.pypipelines/training/finetuning/sft_minimal_evalhub/OWNERSpipelines/training/finetuning/sft_minimal_evalhub/README.mdpipelines/training/finetuning/sft_minimal_evalhub/__init__.pypipelines/training/finetuning/sft_minimal_evalhub/metadata.yamlpipelines/training/finetuning/sft_minimal_evalhub/pipeline.pypipelines/training/finetuning/sft_minimal_evalhub/tests/__init__.pypipelines/training/finetuning/sft_minimal_evalhub/tests/test_pipeline.py
💤 Files with no reviewable changes (4)
- pipelines/training/finetuning/lora/README.md
- components/training/finetuning/lora/README.md
- components/training/finetuning/lora/component.py
- pipelines/training/finetuning/lora/pipeline.py
Comment on lines
+22
to
+24
| PVC_SIZE = "50Gi" | ||
| PVC_STORAGE_CLASS = "nfs-csi" | ||
| PVC_ACCESS_MODES = ["ReadWriteMany"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Hardcoded storage class prevents deployment flexibility.
PVC_STORAGE_CLASS = "nfs-csi" is compile-time constant. Users cannot override it without modifying source code, causing runtime failures in clusters without this storage class. Consider adding a pipeline parameter with "nfs-csi" as the default.
♻️ Proposed fix to parameterize storage class
-PVC_SIZE = "50Gi"
-PVC_STORAGE_CLASS = "nfs-csi"
-PVC_ACCESS_MODES = ["ReadWriteMany"]
-PIPELINE_NAME = "sft-pipeline-evalhub"
+PVC_SIZE = "50Gi"
+PVC_ACCESS_MODES = ["ReadWriteMany"]
+PIPELINE_NAME = "sft-pipeline-evalhub"
# =============================================================================
`@dsl.pipeline`(
name=PIPELINE_NAME,
description="SFT pipeline with Eval Hub evaluation via KServe: benchmarks via Eval Hub, results optionally tracked in MLflow",
- pipeline_config=dsl.PipelineConfig(
- workspace=dsl.WorkspaceConfig(
- size=PVC_SIZE,
- kubernetes=dsl.KubernetesWorkspaceConfig(
- pvcSpecPatch={
- "accessModes": PVC_ACCESS_MODES,
- "storageClassName": PVC_STORAGE_CLASS,
- }
- ),
- ),
- ),
)
def sft_pipeline_evalhub(
+ # PVC configuration
+ pvc_size: str = "50Gi",
+ pvc_storage_class: str = "nfs-csi",
# =========================================================================
# KEY PARAMETERS (Required/Important) - Sorted by stepThen update the @dsl.pipeline decorator to use dsl.PipelineConfig dynamically inside the function body, or use KFP's set_volume task method to configure the PVC at runtime.
🤖 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 `@pipelines/training/finetuning/sft_evalhub/pipeline.py` around lines 22 - 24,
PVC_STORAGE_CLASS is hardcoded which prevents overrides; make it a pipeline
parameter with default "nfs-csi" (e.g., add a function parameter like
pvc_storage_class: str = "nfs-csi" to the pipeline function) and replace uses of
the constant PVC_STORAGE_CLASS with that parameter; keep PVC_SIZE and
PVC_ACCESS_MODES as defaults or convert them similarly if desired, and apply the
runtime value when configuring the PVC (use dsl.PipelineConfig inside the
pipeline or call task.set_volume / the KFP PVC configuration API to set the
storageClassName to the pvc_storage_class parameter so clusters without
"nfs-csi" can pass an alternate class).
Author
There was a problem hiding this comment.
PVC_STORAGE_CLASS is a compile-time constant across all pipelines in this repo (LoRA, SFT, OSFT). KFP workspace PVC config is set at compile time via dsl.PipelineConfig and cannot be parameterized at runtime.
There was a problem hiding this comment.
🧩 Analysis chain
@MStokluska, understood — dsl.PipelineConfig workspace settings are baked in at compile time, so runtime parameterization isn't possible through that API. The suggestion was architecturally invalid for this pattern. I'll record this for future reviews.
🏁 Script executed:
#!/bin/bash
# Resolve the CodeRabbit comment on this PR
gh api graphql -f query='
{
repository(owner: "opendatahub-io", name: "pipelines-components") {
pullRequest(number: 81) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
body
author { login }
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | select(.comments.nodes[0].author.login == "coderabbitai[bot]") | select(.comments.nodes[0].body | contains("PVC_STORAGE_CLASS")) | .id'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Try broader search for the thread ID
gh api graphql -f query='
{
repository(owner: "opendatahub-io", name: "pipelines-components") {
pullRequest(number: 81) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
body
author { login }
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | select(.comments.nodes[0].body | contains("PVC_STORAGE_CLASS")) | .id'
Length of output: 24
🏁 Script executed:
#!/bin/bash
# Resolve the review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: { threadId: "PRRT_kwDORpt8Kc6DfJ1m" }) {
thread {
id
isResolved
}
}
}'
Length of output: 266
@MStokluska, acknowledged — the suggestion was invalid given the KFP compile-time constraint. I wasn't able to resolve the thread programmatically due to permissions, but I've recorded this as a learning to avoid flagging PVC_STORAGE_CLASS, PVC_SIZE, and PVC_ACCESS_MODES as hardcoded values in future reviews of this repo. You can resolve the thread manually.
✏️ Learnings added
Learnt from: MStokluska
Repo: opendatahub-io/pipelines-components PR: 81
File: pipelines/training/finetuning/sft_evalhub/pipeline.py:22-24
Timestamp: 2026-05-20T13:35:25.251Z
Learning: In opendatahub-io/pipelines-components, PVC_STORAGE_CLASS (e.g., "nfs-csi"), PVC_SIZE, and PVC_ACCESS_MODES are intentionally compile-time constants across all pipelines (LoRA, SFT, OSFT, etc.). KFP workspace PVC configuration is set at compile time via `dsl.PipelineConfig` and cannot be parameterized at runtime. Do NOT flag these as hardcoded values that should be made into pipeline parameters.
Learnt from: MStokluska
Repo: opendatahub-io/pipelines-components PR: 81
File: pipelines/training/finetuning/sft_evalhub/pipeline.py:22-24
Timestamp: 2026-05-20T13:35:25.251Z
Learning: In opendatahub-io/pipelines-components, PVC_STORAGE_CLASS (e.g., "nfs-csi"), PVC_SIZE, and PVC_ACCESS_MODES are intentionally compile-time constants across all pipelines (LoRA, SFT, OSFT, etc.). KFP workspace PVC configuration is set at compile time via `dsl.PipelineConfig` and cannot be parameterized at runtime. Do NOT flag these as hardcoded values that should be made into pipeline parameters.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: filip-komarzyniec
Repo: opendatahub-io/pipelines-components PR: 79
File: components/training/autorag/rag_templates_optimization/component.py:104-109
Timestamp: 2026-05-19T10:24:26.943Z
Learning: In this repo’s Python code (opendatahub-io/pipelines-components), do NOT flag bare `httpx.get(...)` calls that omit an explicit `timeout` as a blocking/hanging or resource-exhaustion risk. `httpx` applies a default inactivity timeout of 5 seconds (e.g., `Timeout(timeout=5.0)`) to requests when `timeout` isn’t provided, so missing `timeout` on top-level `httpx.get()` is expected/safe in standard usage.
Learnt from: MStokluska
Repo: opendatahub-io/pipelines-components PR: 81
File: pipelines/training/finetuning/lora_minimal_evalhub/pipeline.py:51-53
Timestamp: 2026-05-20T13:30:50.422Z
Learning: When reviewing pipeline code under `pipelines/training/finetuning/**/pipeline.py`, do not raise an issue for missing dataset URI scheme validation for `dataset_uri` parameters if the pipeline passes `dataset_uri` into the `dataset_download` component (`components/data_processing/dataset_download`), since that component already validates/accepts only `hf://`, `s3://`, and `https://` internally.
| # ========================================================================= | ||
| # KEY PARAMETERS (Required/Important) - Sorted by step | ||
| # ========================================================================= | ||
| phase_01_dataset_man_data_uri: str, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify dataset_download component validates dataset_uri input
# Search for dataset_uri validation in the dataset_download component
rg -nP -A5 -B5 'def dataset_download' --type=py
# Check for URI scheme validation
rg -nP '(scheme|protocol|uri.*valid|path.*travers)' --type=py -g '*dataset_download*'Repository: opendatahub-io/pipelines-components
Length of output: 1343
🏁 Script executed:
cat -n components/data_processing/dataset_download/component.pyRepository: opendatahub-io/pipelines-components
Length of output: 21235
Dataset URI lacks content validation; enables path traversal and SSRF.
dataset_download includes URI format validation (parse_uri, lines 57–85) but this is insufficient. The component accepts and processes arbitrary URI contents without whitelist or containment checks:
- Absolute path traversal (CWE-22): Line 82 accepts URIs like
/etc/passwd; the existence check on line 287 doesn't enforce containment within the intended pvc_mount_path. - S3 SSRF (CWE-918): Line 264 processes any s3:// path using credentials from Kubernetes secrets, allowing pivoting to internal S3 services or unauthorized buckets.
- HTTP SSRF (CWE-918): Line 274 fetches from any https:// URL without hostname validation, enabling requests to internal services (169.254.169.254, localhost, etc.).
Per coding guidelines, training pipelines must validate dataset sources and paths. Add whitelist validation in the pipeline or component before processing untrusted dataset_uri inputs.
🤖 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 `@pipelines/training/finetuning/sft_evalhub/pipeline.py` at line 48, The
pipeline accepts arbitrary phase_01_dataset_man_data_uri and processes it in
dataset_download (uses parse_uri, S3 handling, and HTTP fetch), enabling path
traversal and SSRF; fix by enforcing a strict whitelist and containment checks:
validate scheme against allowed set (e.g., only file:// within pvc_mount_path,
approved s3:// buckets/endpoints, and explicit http(s) hosts), verify parse_uri
hostnames are not local/private IPs (block 127.0.0.0/8, 10.0.0.0/8,
169.254.0.0/16, 192.168.0.0/16, ::1, fc00::/7), require S3 bucket/endpoint
allowlist or exact endpoint match before using Kubernetes credentials, and for
file:/// or bare paths enforce that resolved paths are under the intended
pvc_mount_path (reject any path escaping via .. or absolute paths outside
mount); apply these checks early in dataset_download (using parse_uri output)
and fail fast with clear errors if validation fails.
Author
There was a problem hiding this comment.
The dataset_download component is not part of this PR - it's a pre-existing shared component used by other pipelines in the repo.
There was a problem hiding this comment.
@MStokluska, acknowledged — since components/data_processing/dataset_download is a pre-existing shared component outside the scope of this PR, the findings (CWE-22 path traversal, CWE-918 SSRF) should be tracked separately against that component.
Would you like me to open a GitHub issue to track the security review of the dataset_download component (URI scheme whitelist, SSRF/path-traversal hardening)?
🧠 Learnings used
Learnt from: filip-komarzyniec
Repo: opendatahub-io/pipelines-components PR: 79
File: components/training/autorag/rag_templates_optimization/component.py:104-109
Timestamp: 2026-05-19T10:24:26.943Z
Learning: In this repo’s Python code (opendatahub-io/pipelines-components), do NOT flag bare `httpx.get(...)` calls that omit an explicit `timeout` as a blocking/hanging or resource-exhaustion risk. `httpx` applies a default inactivity timeout of 5 seconds (e.g., `Timeout(timeout=5.0)`) to requests when `timeout` isn’t provided, so missing `timeout` on top-level `httpx.get()` is expected/safe in standard usage.
Learnt from: MStokluska
Repo: opendatahub-io/pipelines-components PR: 81
File: pipelines/training/finetuning/lora_minimal_evalhub/pipeline.py:51-53
Timestamp: 2026-05-20T13:30:50.422Z
Learning: When reviewing pipeline code under `pipelines/training/finetuning/**/pipeline.py`, do not raise an issue for missing dataset URI scheme validation for `dataset_uri` parameters if the pipeline passes `dataset_uri` into the `dataset_download` component (`components/data_processing/dataset_download`), since that component already validates/accepts only `hf://`, `s3://`, and `https://` internally.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/evaluation/evalhub_kserve/tests/test_component_unit.py (1)
50-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing default value assertions for
trust_remote_codeandverify_tls.The test verifies defaults for other parameters but not the newly added security-relevant ones.
Proposed fix
assert params["mlflow_experiment_name"].default == "" assert params["collection_id"].default == "" + assert params["trust_remote_code"].default is False + assert params["verify_tls"].default is False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/evaluation/evalhub_kserve/tests/test_component_unit.py` around lines 50 - 66, The test test_component_default_values is missing assertions for the newly added parameters trust_remote_code and verify_tls on the function signature of evalhub_evaluator_kserve.python_func; update that test to inspect the signature (sig = inspect.signature(evalhub_evaluator_kserve.python_func)) and add assertions that params["trust_remote_code"].default and params["verify_tls"].default equal their intended default values (e.g., False or the actual defaults used in the component) so the unit test covers these security-relevant defaults.pipelines/training/finetuning/sft_evalhub/pipeline.py (1)
271-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAttach
hf-tokento the evaluator task as well.At Line 271, the secret loop excludes
eval_task. Custom benchmark/provider setups that need HF auth will fail in stage 3 even whenhf-tokenexists.Suggested patch
- for _task in [dataset_download_task, training_task]: + for _task in [dataset_download_task, training_task, eval_task]: kfp.kubernetes.use_secret_as_env( task=_task, secret_name="hf-token", secret_key_to_env={"HF_TOKEN": "HF_TOKEN"}, optional=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 `@pipelines/training/finetuning/sft_evalhub/pipeline.py` around lines 271 - 277, The secret "hf-token" is not being attached to the evaluator task; update the secret-binding call so eval_task also receives HF_TOKEN via kfp.kubernetes.use_secret_as_env. Locate the loop that currently iterates over [dataset_download_task, training_task] and either include eval_task in that list or add a separate kfp.kubernetes.use_secret_as_env invocation for eval_task (using the same secret_name "hf-token" and secret_key_to_env {"HF_TOKEN": "HF_TOKEN"}, optional=True) so the evaluator has the required HF credentials.pipelines/training/finetuning/osft_evalhub/pipeline.py (1)
90-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid mutable default for
phase_03_eval_opt_benchmarks.Using a list literal as a default argument can leak mutated state across invocations. Switch to
Noneand initialize inside the function.Proposed fix
- phase_03_eval_opt_benchmarks: list = [ - {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, - ], + phase_03_eval_opt_benchmarks: list | None = None, @@ def osft_pipeline_evalhub( @@ ): + if phase_03_eval_opt_benchmarks is None: + phase_03_eval_opt_benchmarks = [ + {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, + ]🤖 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 `@pipelines/training/finetuning/osft_evalhub/pipeline.py` around lines 90 - 96, The parameter/variable phase_03_eval_opt_benchmarks is currently set to a list literal which is a mutable default; change its default to None and inside the function (or initializer) check "if phase_03_eval_opt_benchmarks is None:" then assign the list [{"id":"leaderboard_ifeval","provider_id":"lm_evaluation_harness"}, ...] so a fresh list is created per call/instance; ensure any references to phase_03_eval_opt_benchmarks use that initialized local variable (not a shared mutable default).
🧹 Nitpick comments (2)
pipelines/training/finetuning/sft_evalhub/pipeline.py (1)
84-90: ⚡ Quick winReplace mutable default
listwithNoneand lazy initialization.Line 84 defines a mutable list as a default function argument. Shared mutable defaults are a Python footgun—each function invocation reuses the same object, creating potential for cross-invocation side effects in tests and pipelined execution.
While the parameter is not currently mutated in this function, the pattern violates Python best practices and poses maintenance risk. Use
Noneas the default and initialize inside the function body.Suggested patch
- phase_03_eval_opt_benchmarks: list = [ - {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, - ], + phase_03_eval_opt_benchmarks: list = None, @@ ): + if phase_03_eval_opt_benchmarks is None: + phase_03_eval_opt_benchmarks = [ + {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, + ]Note: The same pattern appears in 8 other finetuning pipeline files and should be addressed consistently.
🤖 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 `@pipelines/training/finetuning/sft_evalhub/pipeline.py` around lines 84 - 90, The default argument phase_03_eval_opt_benchmarks is set to a mutable list; change its signature to default to None and perform lazy initialization inside the function (e.g., if phase_03_eval_opt_benchmarks is None: phase_03_eval_opt_benchmarks = [ ... ]) so each invocation gets a fresh list; update the function that declares phase_03_eval_opt_benchmarks in pipeline.py (and apply the same None-default + in-body init pattern to the other finetuning pipeline files that use similar list defaults).pipelines/training/finetuning/lora_evalhub/pipeline.py (1)
112-118: ⚡ Quick winAvoid mutable default argument for
phase_03_eval_opt_benchmarks.Using a list literal as a default argument creates shared state across invocations. Replace with
Nonesentinel and instantiate inside the function.Proposed fix
- phase_03_eval_opt_benchmarks: list = [ - {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, - ], + phase_03_eval_opt_benchmarks: list | None = None, @@ ): + if phase_03_eval_opt_benchmarks is None: + phase_03_eval_opt_benchmarks = [ + {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, + ]Note:
evalhub_evaluator_kserveincomponents/evaluation/evalhub_kserve/component.py:29has the same issue withbenchmarks: list = [].🤖 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 `@pipelines/training/finetuning/lora_evalhub/pipeline.py` around lines 112 - 118, The list literal assigned to the parameter/field phase_03_eval_opt_benchmarks creates shared mutable state; change its default from the list literal to None and instantiate the list inside the surrounding function or initializer (e.g., if in a function or dataclass constructor, accept phase_03_eval_opt_benchmarks: Optional[list] = None and set phase_03_eval_opt_benchmarks = [{"id":...}, ...] when None) so each call gets a fresh list; apply the same fix for the benchmarks parameter in evalhub_evaluator_kserve (components/evaluation/evalhub_kserve/component.py) which currently uses benchmarks: list = [].
🤖 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.
Outside diff comments:
In `@components/evaluation/evalhub_kserve/tests/test_component_unit.py`:
- Around line 50-66: The test test_component_default_values is missing
assertions for the newly added parameters trust_remote_code and verify_tls on
the function signature of evalhub_evaluator_kserve.python_func; update that test
to inspect the signature (sig =
inspect.signature(evalhub_evaluator_kserve.python_func)) and add assertions that
params["trust_remote_code"].default and params["verify_tls"].default equal their
intended default values (e.g., False or the actual defaults used in the
component) so the unit test covers these security-relevant defaults.
In `@pipelines/training/finetuning/osft_evalhub/pipeline.py`:
- Around line 90-96: The parameter/variable phase_03_eval_opt_benchmarks is
currently set to a list literal which is a mutable default; change its default
to None and inside the function (or initializer) check "if
phase_03_eval_opt_benchmarks is None:" then assign the list
[{"id":"leaderboard_ifeval","provider_id":"lm_evaluation_harness"}, ...] so a
fresh list is created per call/instance; ensure any references to
phase_03_eval_opt_benchmarks use that initialized local variable (not a shared
mutable default).
In `@pipelines/training/finetuning/sft_evalhub/pipeline.py`:
- Around line 271-277: The secret "hf-token" is not being attached to the
evaluator task; update the secret-binding call so eval_task also receives
HF_TOKEN via kfp.kubernetes.use_secret_as_env. Locate the loop that currently
iterates over [dataset_download_task, training_task] and either include
eval_task in that list or add a separate kfp.kubernetes.use_secret_as_env
invocation for eval_task (using the same secret_name "hf-token" and
secret_key_to_env {"HF_TOKEN": "HF_TOKEN"}, optional=True) so the evaluator has
the required HF credentials.
---
Nitpick comments:
In `@pipelines/training/finetuning/lora_evalhub/pipeline.py`:
- Around line 112-118: The list literal assigned to the parameter/field
phase_03_eval_opt_benchmarks creates shared mutable state; change its default
from the list literal to None and instantiate the list inside the surrounding
function or initializer (e.g., if in a function or dataclass constructor, accept
phase_03_eval_opt_benchmarks: Optional[list] = None and set
phase_03_eval_opt_benchmarks = [{"id":...}, ...] when None) so each call gets a
fresh list; apply the same fix for the benchmarks parameter in
evalhub_evaluator_kserve (components/evaluation/evalhub_kserve/component.py)
which currently uses benchmarks: list = [].
In `@pipelines/training/finetuning/sft_evalhub/pipeline.py`:
- Around line 84-90: The default argument phase_03_eval_opt_benchmarks is set to
a mutable list; change its signature to default to None and perform lazy
initialization inside the function (e.g., if phase_03_eval_opt_benchmarks is
None: phase_03_eval_opt_benchmarks = [ ... ]) so each invocation gets a fresh
list; update the function that declares phase_03_eval_opt_benchmarks in
pipeline.py (and apply the same None-default + in-body init pattern to the other
finetuning pipeline files that use similar list defaults).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7b04e5d5-5ad4-4b4e-9000-2ad18a73ac44
📒 Files selected for processing (25)
components/evaluation/README.mdcomponents/evaluation/evalhub_kserve/OWNERScomponents/evaluation/evalhub_kserve/README.mdcomponents/evaluation/evalhub_kserve/component.pycomponents/evaluation/evalhub_kserve/metadata.yamlcomponents/evaluation/evalhub_kserve/tests/test_component_unit.pypipelines/training/finetuning/README.mdpipelines/training/finetuning/lora_evalhub/README.mdpipelines/training/finetuning/lora_evalhub/metadata.yamlpipelines/training/finetuning/lora_evalhub/pipeline.pypipelines/training/finetuning/lora_minimal_evalhub/README.mdpipelines/training/finetuning/lora_minimal_evalhub/metadata.yamlpipelines/training/finetuning/lora_minimal_evalhub/pipeline.pypipelines/training/finetuning/osft_evalhub/README.mdpipelines/training/finetuning/osft_evalhub/metadata.yamlpipelines/training/finetuning/osft_evalhub/pipeline.pypipelines/training/finetuning/osft_minimal_evalhub/README.mdpipelines/training/finetuning/osft_minimal_evalhub/metadata.yamlpipelines/training/finetuning/osft_minimal_evalhub/pipeline.pypipelines/training/finetuning/sft_evalhub/README.mdpipelines/training/finetuning/sft_evalhub/metadata.yamlpipelines/training/finetuning/sft_evalhub/pipeline.pypipelines/training/finetuning/sft_minimal_evalhub/README.mdpipelines/training/finetuning/sft_minimal_evalhub/metadata.yamlpipelines/training/finetuning/sft_minimal_evalhub/pipeline.py
✅ Files skipped from review due to trivial changes (8)
- components/evaluation/evalhub_kserve/OWNERS
- components/evaluation/README.md
- pipelines/training/finetuning/lora_minimal_evalhub/README.md
- components/evaluation/evalhub_kserve/README.md
- pipelines/training/finetuning/lora_evalhub/README.md
- pipelines/training/finetuning/osft_evalhub/metadata.yaml
- pipelines/training/finetuning/osft_evalhub/README.md
- pipelines/training/finetuning/osft_minimal_evalhub/README.md
Signed-off-by: MStokluska <mstoklus@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pipelines/training/finetuning/sft_evalhub/pipeline.py (1)
84-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace mutable default for
phase_03_eval_opt_benchmarks.Mutable defaults shared across invocations create unnecessary risk. Set
phase_03_eval_opt_benchmarks: list | None = Noneand initialize the benchmark list inside the function body with the same defaults.Proposed fix
- phase_03_eval_opt_benchmarks: list = [ - {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, - {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, - ], + phase_03_eval_opt_benchmarks: list | None = None, @@ - eval_task = evalhub_evaluator_kserve( + benchmarks = phase_03_eval_opt_benchmarks or [ + {"id": "leaderboard_ifeval", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_bbh", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_mmlu_pro", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_musr", "provider_id": "lm_evaluation_harness"}, + {"id": "leaderboard_math_hard", "provider_id": "lm_evaluation_harness"}, + ] + + eval_task = evalhub_evaluator_kserve( @@ - benchmarks=phase_03_eval_opt_benchmarks, + benchmarks=benchmarks,🤖 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 `@pipelines/training/finetuning/sft_evalhub/pipeline.py` around lines 84 - 90, Change the mutable default for phase_03_eval_opt_benchmarks to None by declaring phase_03_eval_opt_benchmarks: list | None = None, and then inside the function body (where phase_03_eval_opt_benchmarks is used) set it to the original list if it is None (i.e., assign the five dicts for "leaderboard_ifeval", "leaderboard_bbh", "leaderboard_mmlu_pro", "leaderboard_musr", "leaderboard_math_hard"); update any type checks or usages to handle the possibly-None parameter.
🧹 Nitpick comments (1)
components/evaluation/evalhub_kserve/tests/test_component_unit.py (1)
85-85: ⚡ Quick winRemove mutable default argument from component signature.
The
benchmarks: list = []parameter uses a mutable default, which can cause shared state issues across function invocations. Change the component default toNone, initialize as[]inside the function body, and update the test assertion on line 85 accordingly.🤖 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 `@components/evaluation/evalhub_kserve/tests/test_component_unit.py` at line 85, Change the mutable default for the component parameter named "benchmarks" from [] to None in the component signature, and inside the component initialize it with "if benchmarks is None: benchmarks = []"; update the unit test in test_component_unit.py that currently asserts params["benchmarks"].default == [] to instead assert that params["benchmarks"].default is None (and keep behavior that the function uses an empty list when called with no argument).
🤖 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.
Outside diff comments:
In `@pipelines/training/finetuning/sft_evalhub/pipeline.py`:
- Around line 84-90: Change the mutable default for phase_03_eval_opt_benchmarks
to None by declaring phase_03_eval_opt_benchmarks: list | None = None, and then
inside the function body (where phase_03_eval_opt_benchmarks is used) set it to
the original list if it is None (i.e., assign the five dicts for
"leaderboard_ifeval", "leaderboard_bbh", "leaderboard_mmlu_pro",
"leaderboard_musr", "leaderboard_math_hard"); update any type checks or usages
to handle the possibly-None parameter.
---
Nitpick comments:
In `@components/evaluation/evalhub_kserve/tests/test_component_unit.py`:
- Line 85: Change the mutable default for the component parameter named
"benchmarks" from [] to None in the component signature, and inside the
component initialize it with "if benchmarks is None: benchmarks = []"; update
the unit test in test_component_unit.py that currently asserts
params["benchmarks"].default == [] to instead assert that
params["benchmarks"].default is None (and keep behavior that the function uses
an empty list when called with no argument).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4fbfa087-533e-43b0-bfa8-f6d34d700140
📒 Files selected for processing (25)
components/evaluation/README.mdcomponents/evaluation/evalhub_kserve/OWNERScomponents/evaluation/evalhub_kserve/README.mdcomponents/evaluation/evalhub_kserve/component.pycomponents/evaluation/evalhub_kserve/metadata.yamlcomponents/evaluation/evalhub_kserve/tests/test_component_unit.pypipelines/training/finetuning/README.mdpipelines/training/finetuning/lora_evalhub/README.mdpipelines/training/finetuning/lora_evalhub/metadata.yamlpipelines/training/finetuning/lora_evalhub/pipeline.pypipelines/training/finetuning/lora_minimal_evalhub/README.mdpipelines/training/finetuning/lora_minimal_evalhub/metadata.yamlpipelines/training/finetuning/lora_minimal_evalhub/pipeline.pypipelines/training/finetuning/osft_evalhub/README.mdpipelines/training/finetuning/osft_evalhub/metadata.yamlpipelines/training/finetuning/osft_evalhub/pipeline.pypipelines/training/finetuning/osft_minimal_evalhub/README.mdpipelines/training/finetuning/osft_minimal_evalhub/metadata.yamlpipelines/training/finetuning/osft_minimal_evalhub/pipeline.pypipelines/training/finetuning/sft_evalhub/README.mdpipelines/training/finetuning/sft_evalhub/metadata.yamlpipelines/training/finetuning/sft_evalhub/pipeline.pypipelines/training/finetuning/sft_minimal_evalhub/README.mdpipelines/training/finetuning/sft_minimal_evalhub/metadata.yamlpipelines/training/finetuning/sft_minimal_evalhub/pipeline.py
✅ Files skipped from review due to trivial changes (12)
- pipelines/training/finetuning/sft_minimal_evalhub/metadata.yaml
- components/evaluation/evalhub_kserve/metadata.yaml
- components/evaluation/README.md
- pipelines/training/finetuning/sft_minimal_evalhub/README.md
- components/evaluation/evalhub_kserve/README.md
- pipelines/training/finetuning/sft_evalhub/README.md
- pipelines/training/finetuning/osft_minimal_evalhub/README.md
- pipelines/training/finetuning/osft_evalhub/README.md
- pipelines/training/finetuning/lora_minimal_evalhub/README.md
- pipelines/training/finetuning/lora_evalhub/README.md
- components/evaluation/evalhub_kserve/OWNERS
- pipelines/training/finetuning/osft_evalhub/metadata.yaml
Fiona-Waters
left a comment
Fiona-Waters
left a comment
There was a problem hiding this comment.
Successfully ran the lora_minimal_evalhub on the team cluster. Looks good to me. Just one question.
| eval_task.set_caching_options(False) | ||
| kfp.kubernetes.set_image_pull_policy(eval_task, "IfNotPresent") | ||
|
|
||
| for _task in [dataset_download_task, training_task]: |
There was a problem hiding this comment.
Would there be any use case where we might need to include eval_task for the hf token?
| eval_task.set_caching_options(False) | ||
| kfp.kubernetes.set_image_pull_policy(eval_task, "IfNotPresent") | ||
|
|
||
| for _task in [dataset_download_task, training_task]: |
There was a problem hiding this comment.
Would there be any use case to include the eval task here too?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes:
Added:
Edits:
Checklist:
Pre-Submission Checklist
Learn more about the pull request title convention used in this repository.
Additional Checklist Items for New or Updated Components/Pipelines
metadata.yamlincludes freshlastVerifiedtimestampare present and complete
snake_casenaming conventionSummary by CodeRabbit
New Features
Changes
Documentation
Tests
Chores