fix(trainer): Fix empty image for Runtime trainer #143

andreyvelich · 2025-11-04T20:21:06Z

As we discussed here, we should move image to the Runtime trainer: #140 (comment)

/assign @kubeflow/kubeflow-sdk-team @Fiona-Waters

coveralls · 2025-11-04T20:22:57Z

Pull Request Test Coverage Report for Build 19113108843

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

22 of 30 (73.33%) changed or added relevant lines in 7 files are covered.
134 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-1.0%) to 67.33%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
kubeflow/trainer/backends/container/utils.py	0	2	0.0%
kubeflow/trainer/backends/container/backend.py	14	20	70.0%

Files with Coverage Reduction	New Missed Lines	%
kubeflow/optimizer/backends/base.py	16	0.0%
kubeflow/optimizer/api/optimizer_client.py	19	0.0%
kubeflow/optimizer/backends/kubernetes/backend.py	99	0.0%

Totals
Change from base Build 19111376131:	-1.0%
Covered Lines:	2504
Relevant Lines:	3719

💛 - Coveralls

andreyvelich · 2025-11-04T20:23:57Z

kubeflow/trainer/backends/container/utils.py

    return UNKNOWN


-def resolve_image(runtime: types.Runtime) -> str:


@Fiona-Waters It looks like we don't need image resolver, since we fallback to the default runtime in case we can't get it online:

sdk/kubeflow/trainer/backends/container/runtime_loader.py

Line 314 in a9c5244

def _create_default_runtimes() -> list[base_types.Runtime]:

Is that correct ?

Yes if image is not optional then we don't need this function.

andreyvelich · 2025-11-04T20:25:29Z

kubeflow/trainer/types/types.py

 class RuntimeTrainer:
    trainer_type: TrainerType
    framework: str
+    image: str


@astefanutti @Fiona-Waters I made image mandatory for the RuntimeTrainer.
The container should always has an image, but for the local subprocess backend, we can populate some const value there.
What do you think about it ?

I think it makes sense for the image to be mandatory. Adding a dummy const to local process backend seems fine to me as long as we make sure it's purpose is mentioned clearly in a comment.

Fiona-Waters

Thanks @andreyvelich - responsed to your comments.

Fiona-Waters · 2025-11-05T10:17:34Z

kubeflow/trainer/backends/container/utils.py

    return UNKNOWN


-def resolve_image(runtime: types.Runtime) -> str:


Yes if image is not optional then we don't need this function.

Fiona-Waters · 2025-11-05T10:19:41Z

kubeflow/trainer/types/types.py

 class RuntimeTrainer:
    trainer_type: TrainerType
    framework: str
+    image: str


I think it makes sense for the image to be mandatory. Adding a dummy const to local process backend seems fine to me as long as we make sure it's purpose is mentioned clearly in a comment.

kramaranya · 2025-11-05T14:49:37Z

/milestone v0.2

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich · 2025-11-05T18:51:29Z

kubeflow/trainer/backends/container/backend.py

+        trainer: Optional[
+            Union[types.CustomTrainer, types.CustomTrainerContainer, types.BuiltinTrainer]
+        ] = None,
+        options: Optional[list] = None,


This should fix E2Es in this PR: kubeflow/trainer#2907

cc @Fiona-Waters @astefanutti @kramaranya

Signed-off-by: Andrey Velichkevich <[email protected]>

kramaranya · 2025-11-05T19:26:08Z

kubeflow/trainer/backends/base.py

        self,
        name: str,
-        follow: Optional[bool] = False,
+        follow: bool = False,


Why do we need this change?

Properties should not be Optional if they can't be None.
By default follow=False

kramaranya · 2025-11-05T20:46:10Z

Thanks @andreyvelich!
/lgtm

andreyvelich · 2025-11-05T20:51:47Z

/approve

google-oss-prow · 2025-11-05T20:51:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andreyvelich]

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

google-oss-prow bot requested review from kramaranya and szaher November 4, 2025 20:21

google-oss-prow bot added the size/L label Nov 4, 2025

andreyvelich commented Nov 4, 2025

View reviewed changes

andreyvelich changed the title ~~fix(sdk): Fix empty image for Runtime trainer~~ fix(trainer): Fix empty image for Runtime trainer Nov 4, 2025

Fiona-Waters reviewed Nov 5, 2025

View reviewed changes

google-oss-prow bot added this to the v0.2 milestone Nov 5, 2025

fix(sdk): Fix empty image for Runtime trainer

f6ab049

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich force-pushed the fix-image-runtime branch from a9c5244 to f6ab049 Compare November 5, 2025 18:50

Add options to container train API

ab87526

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich commented Nov 5, 2025

View reviewed changes

andreyvelich added 3 commits November 5, 2025 18:55

Set name from options in container backend

4ae75dc

Signed-off-by: Andrey Velichkevich <[email protected]>

Fix get_job_logs

432289c

Signed-off-by: Andrey Velichkevich <[email protected]>

Fix test

1323e00

Signed-off-by: Andrey Velichkevich <[email protected]>

kramaranya reviewed Nov 5, 2025

View reviewed changes

google-oss-prow bot assigned kramaranya Nov 5, 2025

google-oss-prow bot added the lgtm label Nov 5, 2025

google-oss-prow bot added the approved label Nov 5, 2025

google-oss-prow bot merged commit 8ebd5ed into kubeflow:main Nov 5, 2025
14 checks passed

andreyvelich deleted the fix-image-runtime branch November 5, 2025 20:52

		return UNKNOWN


		def resolve_image(runtime: types.Runtime) -> str:

fix(trainer): Fix empty image for Runtime trainer #143

fix(trainer): Fix empty image for Runtime trainer #143

Conversation

andreyvelich commented Nov 4, 2025

Uh oh!

coveralls commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19113108843

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fiona-Waters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kramaranya commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kramaranya commented Nov 5, 2025

Uh oh!

andreyvelich commented Nov 5, 2025

Uh oh!

google-oss-prow bot commented Nov 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Nov 4, 2025 •

edited

Loading