fix: improve logging around packages_to_install by briangallagher · Pull Request #269 · kubeflow/sdk

briangallagher · 2026-02-06T17:08:36Z

Improve pip install logging for runtime package installation #268

Fixes #268

Checklist:

Docs included if any changes are user facing

Signed-off-by: Brian Gallagher <briangal@gmail.com>

kramaranya

Thanks @briangallagher, left a few comments.
/assign @andreyvelich @astefanutti

kramaranya · 2026-02-11T10:11:33Z

kubeflow/trainer/backends/kubernetes/utils.py

+            echo "ERROR: Failed to install Python packages: $PACKAGES" >&2
+            cat "$LOG_FILE" >&2


shall we have exit 1 here?

I think we should, updated.

kramaranya · 2026-02-11T10:15:47Z

kubeflow/trainer/backends/kubernetes/utils.py

+        if PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet \\
+            --no-warn-script-location $PIP_OPTS --user $PACKAGES >"$LOG_FILE" 2>&1; then
+            echo "Successfully installed Python packages: $PACKAGES"
+        elif PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet \\
+            --no-warn-script-location $PIP_OPTS $PACKAGES >"$LOG_FILE" 2>&1; then
+            echo "Successfully installed Python packages: $PACKAGES"
+        else
+            echo "ERROR: Failed to install Python packages: $PACKAGES" >&2
+            cat "$LOG_FILE" >&2


Looks like we overwrite the first attempt's output with the second. Can we append the second output?

Good catch, updated now

Signed-off-by: Brian Gallagher <briangal@gmail.com>

google-oss-prow · 2026-02-18T08:58:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

coveralls · 2026-02-18T09:01:05Z

Pull Request Test Coverage Report for Build 22133084545

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
429 unchanged lines in 18 files lost coverage.
Overall coverage increased (+9.1%) to 76.639%

Files with Coverage Reduction	New Missed Lines	%
kubeflow/common/utils.py	3	61.11%
kubeflow/optimizer/types/search_types.py	3	88.0%
kubeflow/trainer/backends/base.py	3	70.59%
kubeflow/hub/api/model_registry_client.py	4	92.16%
kubeflow/trainer/backends/container/utils.py	10	83.02%
kubeflow/trainer/backends/kubernetes/backend_test.py	10	96.66%
kubeflow/trainer/api/trainer_client.py	13	68.29%
kubeflow/optimizer/backends/kubernetes/utils.py	16	48.08%
kubeflow/trainer/backends/container/adapters/base.py	16	69.23%
kubeflow/trainer/backends/container/backend_test.py	18	93.55%

Totals
Change from base Build 21720559217:	9.1%
Covered Lines:	4301
Relevant Lines:	5612

💛 - Coveralls

Copilot

Pull request overview

This PR improves pip install logging for runtime package installation to address issue #268. The previous implementation ran two pip install attempts with ||, causing confusing error messages when the first attempt failed but the second succeeded. The new implementation uses explicit shell logic with an if-elif-else structure that captures pip output to a log file and only displays errors when both installation attempts fail.

Changes:

Refactored pip install script to use explicit conditional logic instead of || operator
Added log file capture (/tmp/pip_install.log) to store pip output from both installation attempts
Success cases now print concise confirmation messages instead of verbose pip output
Error cases display captured log content only when both user and system-wide installations fail

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`kubeflow/trainer/backends/kubernetes/utils.py`	Rewrote `get_script_for_python_packages` to use shell variables and if-elif-else logic for better error handling
`kubeflow/trainer/backends/kubernetes/utils_test.py`	Updated test expectations to match new shell script format with log file handling
`kubeflow/trainer/backends/kubernetes/backend_test.py`	Refactored `get_custom_trainer` to call `utils.get_script_for_python_packages` for consistency with production code

Copilot · 2026-02-18T09:02:47Z

kubeflow/trainer/backends/kubernetes/utils.py

    # first url will be the index-url.
    options = [f"--index-url {pip_index_urls[0]}"]
    options.extend(f"--extra-index-url {extra_index_url}" for extra_index_url in pip_index_urls[1:])
+    options_str = " ".join(options)


Command injection vulnerability: pip_index_urls are embedded in shell options without escaping. A malicious URL like 'https://evil.com"; rm -rf / #' could break out of the quotes and execute arbitrary commands. Apply shlex.quote() to each URL when building options: options = [f"--index-url {shlex.quote(pip_index_urls[0])}"] and similarly for extra-index-url.

Good catch, tho.
@kubeflow/kubeflow-sdk-team @kubeflow/wg-pipeline-leads Do we know if we have the same CVE in KFP Client upstream?

Copilot · 2026-02-18T09:02:48Z

kubeflow/trainer/backends/kubernetes/utils.py

+        PACKAGES="{packages_str}"
+        PIP_OPTS="{options_str}"


Command injection vulnerability: packages_str and options_str are directly interpolated into double-quoted shell strings without escaping. A malicious package name like 'torch"; rm -rf / #' or URL with shell metacharacters could break out of the quotes and execute arbitrary commands. Use shlex.quote() to properly escape each package name and URL before joining them, or use shell arrays for safer variable handling.

astefanutti · 2026-02-18T10:05:16Z

Thanks @briangallagher!

/lgtm

kramaranya

Thanks @briangallagher!
/lgtm
/assign @andreyvelich @Fiona-Waters

andreyvelich

Sorry for the delay @briangallagher!
Overall, looks good! I left a few thoughts.

andreyvelich · 2026-02-20T02:13:16Z

kubeflow/trainer/backends/kubernetes/utils.py

    # first url will be the index-url.
    options = [f"--index-url {pip_index_urls[0]}"]
    options.extend(f"--extra-index-url {extra_index_url}" for extra_index_url in pip_index_urls[1:])
+    options_str = " ".join(options)


Good catch, tho.
@kubeflow/kubeflow-sdk-team @kubeflow/wg-pipeline-leads Do we know if we have the same CVE in KFP Client upstream?

andreyvelich · 2026-02-20T02:16:18Z

kubeflow/trainer/backends/kubernetes/utils.py

+
+        if PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet \\
+            --no-warn-script-location $PIP_OPTS --user $PACKAGES >"$LOG_FILE" 2>&1; then
+            echo "Successfully installed Python packages: $PACKAGES"


Maybe we should say?

Suggested change

echo "Successfully installed Python packages: $PACKAGES"

echo "Successfully installed the user' Python packages: $PACKAGES"

andreyvelich · 2026-02-20T02:16:38Z

kubeflow/trainer/backends/kubernetes/utils.py

+            echo "Successfully installed Python packages: $PACKAGES"
+        elif PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet \\
+            --no-warn-script-location $PIP_OPTS $PACKAGES >>"$LOG_FILE" 2>&1; then
+            echo "Successfully installed Python packages: $PACKAGES"


Suggested change

echo "Successfully installed Python packages: $PACKAGES"

echo "Successfully installed the system's Python packages: $PACKAGES"

andreyvelich · 2026-02-20T02:17:01Z

kubeflow/trainer/backends/kubernetes/utils.py

+        f"""
+        PACKAGES="{packages_str}"
+        PIP_OPTS="{options_str}"
+        LOG_FILE=/tmp/pip_install.log


What if /tmp directory doesn't exist, shall we just use ?
Do we know if KFP client write pip logs to a file?
cc @kubeflow/wg-pipeline-leads

google-oss-prow bot requested review from Electronic-Waste, kramaranya and szaher February 6, 2026 17:08

google-oss-prow bot added the size/M label Feb 6, 2026

improve logging around packages_to_install

363a68f

Signed-off-by: Brian Gallagher <briangal@gmail.com>

briangallagher force-pushed the improve-logging branch from ed465a1 to 363a68f Compare February 7, 2026 11:08

google-oss-prow bot added size/L and removed size/M labels Feb 7, 2026

briangallagher changed the title ~~improve logging around packages_to_install~~ feat: improve logging around packages_to_install Feb 10, 2026

briangallagher changed the title ~~feat: improve logging around packages_to_install~~ fix: improve logging around packages_to_install Feb 10, 2026

kramaranya reviewed Feb 11, 2026

View reviewed changes

google-oss-prow bot assigned andreyvelich and astefanutti Feb 11, 2026

exit when pip install fails, append errors from both attempts

f009677

Signed-off-by: Brian Gallagher <briangal@gmail.com>

Copilot AI review requested due to automatic review settings February 18, 2026 08:58

Copilot started reviewing on behalf of briangallagher February 18, 2026 08:58 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

google-oss-prow bot added the lgtm label Feb 18, 2026

kramaranya reviewed Feb 18, 2026

View reviewed changes

google-oss-prow bot assigned Fiona-Waters and kramaranya Feb 18, 2026

andreyvelich reviewed Feb 20, 2026

View reviewed changes

		echo "ERROR: Failed to install Python packages: $PACKAGES" >&2
		cat "$LOG_FILE" >&2

	echo "Successfully installed Python packages: $PACKAGES"
	echo "Successfully installed the user' Python packages: $PACKAGES"

	echo "Successfully installed Python packages: $PACKAGES"
	echo "Successfully installed the system's Python packages: $PACKAGES"

Comments

Conversation

briangallagher commented Feb 6, 2026

Uh oh!

kramaranya left a comment

Choose a reason for hiding this comment

Uh oh!

kramaranya Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

briangallagher Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

kramaranya Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

briangallagher Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

google-oss-prow bot commented Feb 18, 2026

Uh oh!

coveralls commented Feb 18, 2026

Pull Request Test Coverage Report for Build 22133084545

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

andreyvelich Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

astefanutti commented Feb 18, 2026

Uh oh!

kramaranya left a comment

Choose a reason for hiding this comment

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

andreyvelich Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvelich Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

andreyvelich Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

andreyvelich Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andreyvelich Feb 20, 2026 •

edited

Loading

andreyvelich Feb 20, 2026 •

edited

Loading