feat: update pipeline components to use latest universal image#97
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR updates container base images in multiple components to quay.io/opendatahub/odh-th06-cpu-torch291-py312:odh-3.4 and updates the allowlist. The lm_eval component changes imports to use TaskConfig and Task from lm_eval.api.task and adjusts a custom Task subclass. LoRA, OSFT, and SFT training wrappers now accept keyword arguments (**p) and forward them to their respective training_hub functions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Security & Quality Issues
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/evaluation/lm_eval/component.py (2)
534-556:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDefault
trust_remote_codetoFalseto prevent RCE during model loading.
trust_remote_code=Trueinstructs Transformers to download and execute arbitrary Python from the model repository/artifact duringfrom_pretrained; iffinal_model_pathis user-controlled, this enables code execution in the evaluation pod (CWE-829 / CWE-94). Default toFalseand only allow remote code behind an explicit, trusted opt-in (optionally with allowlist + pinned revision).Suggested fix
- hf_model_args = { - "pretrained": final_model_path, - "trust_remote_code": True, - "dtype": "auto", - } + hf_model_args = { + "pretrained": final_model_path, + "trust_remote_code": False, + "dtype": "auto", + }🤖 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/lm_eval/component.py` around lines 534 - 556, The hf_model_args currently sets "trust_remote_code": True which permits remote code execution when loading models; change the default to "trust_remote_code": False in the hf_model_args construction and ensure any path that sets it to True requires an explicit, documented opt-in (e.g., a config flag or parameter) before it's merged into hf_model_args; update the logic near hf_model_args, final_model_path, and where model_class.create_from_arg_obj is called (get_model("hf") / model_class) so only an explicit trusted flag can override the False default (optionally add an allowlist/pinned revision check before permitting True).
413-421:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip local
config.jsonfilesystem validation for HF Hub model IDs
model_pathis documented to accept Hub IDs (e.g.,ibm/granite-7b), and later the component passesfinal_model_pathas HFpretrained; but lines ~413-421 unconditionally buildos.path.join(final_model_path, "config.json")and fail early for non-existent “local” paths, blocking legitimate Hub resolution.Suggested fix
- # Verify model directory has config.json (required by HF) - config_path = os.path.join(final_model_path, "config.json") - if not os.path.exists(config_path): - logger.error(f"Model directory missing config.json: {final_model_path}") - if os.path.isdir(final_model_path): - logger.error(f"Directory contents: {os.listdir(final_model_path)}") - else: - logger.error("Path is NOT A DIRECTORY") - raise ValueError(f"Invalid model directory - no config.json found at {final_model_path}") + # Only local model directories can be validated via the filesystem here. + if os.path.exists(final_model_path): + if not os.path.isdir(final_model_path): + raise ValueError(f"Invalid model path: {final_model_path} is not a directory") + config_path = os.path.join(final_model_path, "config.json") + if not os.path.exists(config_path): + logger.error(f"Model directory missing config.json: {final_model_path}") + logger.error(f"Directory contents: {os.listdir(final_model_path)}") + raise ValueError(f"Invalid model directory - no config.json found at {final_model_path}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/evaluation/lm_eval/component.py` around lines 413 - 421, The current validation always treats final_model_path as a local filesystem directory and fails when a HF Hub model ID is used; change the logic in the block that builds config_path (using final_model_path, config_path, logger) so it only performs the local filesystem check when final_model_path actually exists on disk (e.g., os.path.exists(final_model_path) and os.path.isdir(final_model_path)); if the path does not exist locally, skip the local config.json check and allow the HF Hub identifier to be passed through to the HF loader instead.
🤖 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/lm_eval/component.py`:
- Line 14: The component unconditionally enables remote-code execution and
incorrectly requires a local config.json for HF Hub IDs; fix by defaulting
hf_model_args["trust_remote_code"] = False and only set it True when an explicit
trusted opt-in (e.g., m_args.get("trust_remote_code") is True) is provided;
change the config.json existence check around final_model_path so it only
validates config.json if os.path.isdir(final_model_path) (skip the check when
final_model_path is a non-local HF Hub ID and let the HF backend resolve it);
and pin the lm-eval dependency instead of installing the unpinned "lm-eval" (use
the tested exact version and include HF extras if needed since get_model("hf")
is used). Reference symbols: hf_model_args, m_args, final_model_path, and the
top-level lm-eval installation entry.
In `@scripts/validate_base_images/base_image_allowlist.yaml`:
- Line 13: Update the allowlist and source files to require immutable digests:
replace the tag-based entry
'quay.io/opendatahub/odh-th06-cpu-torch291-py312:odh-3.4' in
base_image_allowlist.yaml with the approved digest form
quay.io/opendatahub/odh-th06-cpu-torch291-py312@sha256:<approved_digest>, change
the hardcoded base_image literals in
components/data_processing/dataset_download/component.py,
components/deployment/kubeflow_model_registry/component.py,
components/training/finetuning/{lora,osft,sft}/component.py to use the same
`@sha256` digest form, and remove or tighten the tag-matching
allowed_image_patterns entry (the regex
^quay\.io/opendatahub/odh-[\w-]+:odh-.+$) so the validator
(validate_base_images) no longer accepts mutable tag-based images.
---
Outside diff comments:
In `@components/evaluation/lm_eval/component.py`:
- Around line 534-556: The hf_model_args currently sets "trust_remote_code":
True which permits remote code execution when loading models; change the default
to "trust_remote_code": False in the hf_model_args construction and ensure any
path that sets it to True requires an explicit, documented opt-in (e.g., a
config flag or parameter) before it's merged into hf_model_args; update the
logic near hf_model_args, final_model_path, and where
model_class.create_from_arg_obj is called (get_model("hf") / model_class) so
only an explicit trusted flag can override the False default (optionally add an
allowlist/pinned revision check before permitting True).
- Around line 413-421: The current validation always treats final_model_path as
a local filesystem directory and fails when a HF Hub model ID is used; change
the logic in the block that builds config_path (using final_model_path,
config_path, logger) so it only performs the local filesystem check when
final_model_path actually exists on disk (e.g., os.path.exists(final_model_path)
and os.path.isdir(final_model_path)); if the path does not exist locally, skip
the local config.json check and allow the HF Hub identifier to be passed through
to the HF loader instead.
🪄 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: 717294d5-151a-4123-921e-7d1677c09277
📒 Files selected for processing (7)
components/data_processing/dataset_download/component.pycomponents/deployment/kubeflow_model_registry/component.pycomponents/evaluation/lm_eval/component.pycomponents/training/finetuning/lora/component.pycomponents/training/finetuning/osft/component.pycomponents/training/finetuning/sft/component.pyscripts/validate_base_images/base_image_allowlist.yaml
| base_image="registry.access.redhat.com/ubi9/python-311:latest", | ||
| packages_to_install=[ | ||
| "lm-eval[vllm]", # The core harness with vLLM backend | ||
| "lm-eval", # The core harness (CPU/HF backend — no ray/vLLM dependency) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest lm-evalrelease on PyPI, and do its release notes mention breaking changes toTask imports or Hugging Face backend loading?
💡 Result:
Latest lm-eval release on PyPI (as of 2026-05-26): v0.4.12, dated 2026-05-11. [1] Do the release notes mention breaking changes to Task imports? No. The Task-import-breaking changes are not mentioned in the available release-note snippet for v0.4.12; the v0.4.12 release notes we found focus on fixes/improvements and do not call out Task import path changes. [2] Do the release notes mention breaking changes to Hugging Face backend loading? No for v0.4.12 specifically. However, a previous release (v0.4.10) contains a breaking-change note that the base pip install lm_eval no longer installs the Hugging Face/torch stack by default, and that Hugging Face backend support requires installing extras (pip install lm_eval[hf]). [3] So: v0.4.12 is the latest PyPI release, and its release notes (based on what we could retrieve) do not mention breaking Task imports or Hugging Face backend loading changes, though Hugging Face backend loading changes were explicitly called out in v0.4.10. [1][2][3]
Citations:
- 1: https://pypi.org/project/lm-eval/
- 2: https://github.com/EleutherAI/lm-evaluation-harness/releases
- 3: https://github.com/EleutherAI/lm-evaluation-harness/releases/tag/v0.4.10
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/evaluation/lm_eval/component.py"
# Show file with line numbers around the referenced sections
nl -ba "$FILE" | sed -n '1,180p'
nl -ba "$FILE" | sed -n '380,470p'
nl -ba "$FILE" | sed -n '480,620p'Repository: opendatahub-io/pipelines-components
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/evaluation/lm_eval/component.py"
nl -ba "$FILE" | sed -n '1,180p'
nl -ba "$FILE" | sed -n '380,470p'
nl -ba "$FILE" | sed -n '480,620p'Repository: opendatahub-io/pipelines-components
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="components/evaluation/lm_eval/component.py"
# Print with line numbers using cat -n (nl is unavailable in the sandbox)
cat -n "$FILE" | sed -n '1,220p'
cat -n "$FILE" | sed -n '360,470p'
cat -n "$FILE" | sed -n '500,620p'Repository: opendatahub-io/pipelines-components
Length of output: 20559
Critical: Remove unconditional remote-code execution and fix HF Hub ID loading
- Security (Critical, CWE-94):
components/evaluation/lm_eval/component.pysetstrust_remote_code=Trueunconditionally when buildinghf_model_args(lines 534-537). Ifmodel_path/model_artifactis user-controlled and points to a malicious HF repo, model loading can execute arbitrary code (RCE). Default toFalseand require an explicit opt-in for trusted sources (e.g., trusted local dirs). Example:hf_model_args = { "pretrained": final_model_path, "trust_remote_code": False, # default "dtype": "auto", } # optionally allow remote code only via an explicit, trusted flag/input if m_args.get("trust_remote_code") is True: hf_model_args["trust_remote_code"] = True
- Correctness: The documented
model_pathsupports HF Hub IDs (e.g.,"ibm/granite-7b"), but theconfig.jsonfilesystem check enforces a local directory (final_model_path/config.json) and raisesValueErrorfor non-local IDs (lines 413-421). Only requireconfig.jsonwhenfinal_model_pathis an existing local directory; otherwise skip and let the HF backend resolve the Hub model. - Reproducibility: Pin
lm-evalinstead of installing unpinned"lm-eval"(line ~14), since upstream API drift is already handled; use the exact tested version (e.g.,lm-eval==0.4.12) and consider installing HF extras (e.g.,lm-eval[hf]) since the component loadsget_model("hf").
🤖 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/lm_eval/component.py` at line 14, The component
unconditionally enables remote-code execution and incorrectly requires a local
config.json for HF Hub IDs; fix by defaulting hf_model_args["trust_remote_code"]
= False and only set it True when an explicit trusted opt-in (e.g.,
m_args.get("trust_remote_code") is True) is provided; change the config.json
existence check around final_model_path so it only validates config.json if
os.path.isdir(final_model_path) (skip the check when final_model_path is a
non-local HF Hub ID and let the HF backend resolve it); and pin the lm-eval
dependency instead of installing the unpinned "lm-eval" (use the tested exact
version and include HF extras if needed since get_model("hf") is used).
Reference symbols: hf_model_args, m_args, final_model_path, and the top-level
lm-eval installation entry.
| allowed_images: | ||
| - 'quay.io/opendatahub/odh-training-th04-cpu-torch29-py312-rhel9:cpu-3.3' | ||
| - 'registry.access.redhat.com/ubi9/python-311:latest' | ||
| - 'quay.io/opendatahub/odh-th06-cpu-torch291-py312:odh-3.4' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tag-based references in touched files:"
rg -n 'odh-th06-cpu-torch291-py312:odh-3\.4' \
scripts/validate_base_images/base_image_allowlist.yaml \
components/data_processing/dataset_download/component.py \
components/deployment/kubeflow_model_registry/component.py \
components/training/finetuning/lora/component.py \
components/training/finetuning/osft/component.py \
components/training/finetuning/sft/component.py
echo
echo "Digest-pinned references in touched files (expected after fix):"
rg -n '`@sha256`:' \
scripts/validate_base_images/base_image_allowlist.yaml \
components/data_processing/dataset_download/component.py \
components/deployment/kubeflow_model_registry/component.py \
components/training/finetuning/lora/component.py \
components/training/finetuning/osft/component.py \
components/training/finetuning/sft/component.py || trueRepository: opendatahub-io/pipelines-components
Length of output: 950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate allowlist loader + validator:"
rg -n "load_base_image_allowlist|is_valid_base_image|validate_base_images\.py" -S \
scripts/validate_base_images || true
echo
echo "Show implementation details (if present):"
rg -n "def load_base_image_allowlist|def is_valid_base_image" -S \
scripts/validate_base_images/validate_base_images.py || true
echo
echo "Print the allowlist-loading snippet area (around matched lines):"
rg -n "load_base_image_allowlist" -n scripts/validate_base_images/validate_base_images.py -C 3 || trueRepository: opendatahub-io/pipelines-components
Length of output: 8766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find lib/base_image implementation files:"
fd -a "base_image*" scripts/validate_base_images -t f
echo
echo "Locate load_base_image_allowlist + is_valid_base_image definitions:"
rg -n "def load_base_image_allowlist|def is_valid_base_image" -S \
scripts/validate_base_images/lib || true
echo
echo "Show the relevant implementation blocks (around definitions):"
for f in $(rg -l "def load_base_image_allowlist|def is_valid_base_image" -S scripts/validate_base_images/lib); do
echo "---- $f ----"
rg -n "def load_base_image_allowlist|def is_valid_base_image" -n "$f" -C 3
done
echo
echo "Check tests for digest usage/behavior:"
rg -n "`@sha256`:|digest|digests" scripts/validate_base_images/tests/test_validate_base_images.py -n || trueRepository: opendatahub-io/pipelines-components
Length of output: 686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for BaseImageAllowlist / load_base_image_allowlist definitions across repo:"
rg -n "BaseImageAllowlist|load_base_image_allowlist|is_valid_base_image" -S . || true
echo
echo "Show where base_image.py lives (if any):"
fd -i "base_image.py" . || true
echo
echo "Search for '`@sha256`:' handling in code:"
rg -n "`@sha256`:|digest(s)?|sha256" -S . || true
echo
echo "Inspect allowlist matcher logic by opening candidate file(s):"
for f in $(rg -l "load_base_image_allowlist|is_valid_base_image" -S .); do
echo "---- $f ----"
sed -n '1,220p' "$f" | sed -n '1,220p' || true
doneRepository: opendatahub-io/pipelines-components
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== scripts/validate_base_images/base_image_allowlist.yaml (top) ==="
sed -n '1,120p' scripts/validate_base_images/base_image_allowlist.yaml
echo
echo "=== scripts/lib/base_image.py: show load_base_image_allowlist + matcher ==="
sed -n '1,260p' scripts/lib/base_image.py
echo
echo "=== scripts/lib/base_image.py: show is_valid_base_image ==="
sed -n '260,420p' scripts/lib/base_image.py
echo
echo "=== scripts/validate_base_images/tests/test_validate_base_images.py: digest-related assertions ==="
rg -n "sha256:|digest" scripts/validate_base_images/tests/test_validate_base_images.py || true
echo
echo "=== scripts/validate_base_images/tests/test_validate_base_images.py: allowlist parsing behavior ==="
rg -n "load_base_image_allowlist|BaseImageAllowlist|allowlist" scripts/validate_base_images/tests/test_validate_base_images.py -n | head -n 80Repository: opendatahub-io/pipelines-components
Length of output: 21121
Pin allowlisted base images to immutable digests (CWE-494, major).
The validator allows base images via exact string match / regex match only (image in allowed_images or allowed_image_patterns), with no digest/content integrity enforcement—so mutable tags like quay.io/opendatahub/odh-th06-cpu-torch291-py312:odh-3.4 will keep passing even if upstream retags.
Exploit scenario: the registry retags :odh-3.4 (or attacker swaps bytes for that tag) and component runtime images change while validate_base_images still accepts the original tag string.
Remediation code
allowed_images:
- - 'quay.io/opendatahub/odh-th06-cpu-torch291-py312:odh-3.4'
+ - 'quay.io/opendatahub/odh-th06-cpu-torch291-py312@sha256:<approved_digest>'Update the base_image="..." literals currently using the tag:
components/data_processing/dataset_download/component.pycomponents/deployment/kubeflow_model_registry/component.pycomponents/training/finetuning/lora/component.pycomponents/training/finetuning/osft/component.pycomponents/training/finetuning/sft/component.py
to quay.io/opendatahub/odh-th06-cpu-torch291-py312@sha256:<approved_digest>.
Also remove/replace the tag-based allowed_image_patterns entry that matches ^quay\.io/opendatahub/odh-[\w-]+:odh-.+$ (otherwise unrelated retags can still pass via the regex).
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @scripts/validate_base_images/base_image_allowlist.yaml at line 13, Update
the allowlist and source files to require immutable digests: replace the
tag-based entry 'quay.io/opendatahub/odh-th06-cpu-torch291-py312:odh-3.4' in
base_image_allowlist.yaml with the approved digest form
quay.io/opendatahub/odh-th06-cpu-torch291-py312@sha256:<approved_digest>, change
the hardcoded base_image literals in
components/data_processing/dataset_download/component.py,
components/deployment/kubeflow_model_registry/component.py,
components/training/finetuning/{lora,osft,sft}/component.py to use the same
@sha256 digest form, and remove or tighten the tag-matching
allowed_image_patterns entry (the regex
^quay.io/opendatahub/odh-[\w-]+:odh-.+$) so the validator
(validate_base_images) no longer accepts mutable tag-based images.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Fiona-Waters
left a comment
Fiona-Waters
left a comment
There was a problem hiding this comment.
Thanks @Slowlybomb
I tested this with the lora_minimal pipeline, the changes in the dataset_download, kubeflow_model_registry and lora components work as expected. I have left comments for the rest.
| # - allowed_image_patterns: list of regex patterns matched against the full image | ||
| allowed_images: | ||
| - 'quay.io/opendatahub/odh-training-th04-cpu-torch29-py312-rhel9:cpu-3.3' | ||
| - 'registry.access.redhat.com/ubi9/python-311:latest' |
There was a problem hiding this comment.
I don't think 'registry.access.redhat.com/ubi9/python-311:latest' should be removed?
There was a problem hiding this comment.
Yeah, I will put it back. Should had ask about it before removing.
| from lm_eval.evaluator import evaluate | ||
| from lm_eval.tasks import get_task_dict | ||
|
|
||
| # lm-eval >= 0.4 moved Task out of the tasks module — patch it back for compatibility |
There was a problem hiding this comment.
I don't think that this is needed. After adding the Task import you can just use that directly here: https://github.com/opendatahub-io/pipelines-components/blob/main/components/evaluation/lm_eval/component.py#L170 as class ChatHoldoutTask(Task)
There was a problem hiding this comment.
I will trust you on this. But I am still worrying, that import can brake. So it is nice to have explanation
There was a problem hiding this comment.
Apologies if I wasn't clear here, I am not asking for you to remove the comment only but the way that you have used Task.
| base_image="registry.access.redhat.com/ubi9/python-311:latest", | ||
| packages_to_install=[ | ||
| "lm-eval[vllm]", # The core harness with vLLM backend | ||
| "lm-eval", # The core harness (CPU/HF backend — no ray/vLLM dependency) |
There was a problem hiding this comment.
We shouldn't change this. It should be the vllm backend. I think we need to change the image that is used in this component, possibly to our 3.4 cuda image but that would need to be tested. When I ran it, I got errors related to missing cuda libs. This might be outside the scope of this PR.
There was a problem hiding this comment.
if I am not mistaken, this was problem with Ray, that was required for vllm, so I had to remove it.
So just return [vllm]?
There was a problem hiding this comment.
Removing vllm changes the way in which this component works, which we don't want. You have switched from using GPU to using CPU - but this component is meant to run on GPU. We could try this with the cuda image, instead of the cpu one. This could possibly be done in a separate PR, but in this PR we should revert these changes that update lm-eval not to use vllm.
Slowlybomb
force-pushed
the
RHOAIENG-56970-update-universal-image
branch
from
c4f3db5 to
36a6831
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lm_eval/component.py`:
- Around line 79-87: The import block is unsorted causing Ruff I001; reorder
imports so stdlib (if any) come first, then third-party "import torch", then the
delayed lm_eval imports (e.g., from lm_eval.api.instance import Instance, from
lm_eval.api.metrics import mean, from lm_eval.api.registry import get_model,
from lm_eval.api.task import TaskConfig, Task, from lm_eval.evaluator import
evaluate, from lm_eval.tasks import get_task_dict), or simply run "ruff check
--fix" to auto-fix the ordering.
- Line 85: The project currently imports TaskConfig and Task from
lm_eval.api.task but the dependency lm-eval[vllm] is installed unpinned; update
the package entry in the packages_to_install list to pin lm-eval[vllm] to an
exact published version (e.g. "lm-eval[vllm]==X.Y.Z") and preferably include the
corresponding pip hash(s) to mitigate supply-chain/CWE-494 risks; ensure any
CI/deployment scripts or setup code that references packages_to_install (and any
requirement files) are updated to use the pinned spec so the TaskConfig/Task API
surface remains stable.
🪄 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: 70351b48-5484-4fb4-8e39-191d4ccbac04
📒 Files selected for processing (7)
components/data_processing/dataset_download/component.pycomponents/deployment/kubeflow_model_registry/component.pycomponents/evaluation/lm_eval/component.pycomponents/training/finetuning/lora/component.pycomponents/training/finetuning/osft/component.pycomponents/training/finetuning/sft/component.pyscripts/validate_base_images/base_image_allowlist.yaml
✅ Files skipped from review due to trivial changes (2)
- components/data_processing/dataset_download/component.py
- components/deployment/kubeflow_model_registry/component.py
🚧 Files skipped from review as they are similar to previous changes (3)
- components/training/finetuning/osft/component.py
- components/training/finetuning/sft/component.py
- components/training/finetuning/lora/component.py
Fiona-Waters
left a comment
Fiona-Waters
left a comment
There was a problem hiding this comment.
Just a couple more tiny things.
To fix the lint check failure run ruff check --fix components/evaluation/lm_eval/component.py on your local machine.
| @@ -236,7 +236,7 @@ def _params() -> Dict: | |||
|
|
|||
| params = _params() | |||
|
|
|||
| def _train_func(p): | |||
| def _train_func(**p): | |||
| a = dict(p or {}) | |||
There was a problem hiding this comment.
Can you update this to match the other please?
| a = dict(p or {}) | |
| a = dict(p) |
There was a problem hiding this comment.
Could you remove this line? self.task_name = task_name
There was a problem hiding this comment.
And this one also? self.config.task = task_name they are not required based on the other Task related changes.
Slowlybomb
force-pushed
the
RHOAIENG-56970-update-universal-image
branch
3 times, most recently
from
64bc663 to
c4b1ce4
Compare
- bump base_image to odh-th06-cpu-torch291-py312:odh-3.4 across training, data processing, and deployment components - fix _train_func signature to use **kwargs in lora, osft, sft - fix lm_eval Task import for direct usage (from lm_eval.api.task) - remove unnecessary self.task_name and self.config.task assignments - regenerate deployment category README after upstream rebase Signed-off-by: Slowlybomb <hslyusar@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Slowlybomb
force-pushed
the
RHOAIENG-56970-update-universal-image
branch
from
c4b1ce4 to
aa2894c
Compare
Fiona-Waters
left a comment
Fiona-Waters
left a comment
There was a problem hiding this comment.
Ran the sft minimal pipeline to test, works as expected. Follow on issue required to update the image in the lm_eval component. Thanks for your work on this @Slowlybomb 🎉
/lgtm
/approve
|
@nsingla we need to merge this but I don't seem to have permissions any more. Can you help please? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Fiona-Waters, nsingla The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
openshift-merge-bot
Bot
merged commit 71547ab
into
opendatahub-io:main
3 participants
Description of your changes:
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
Chores
Refactor