Patch elyra kfp template to fix kfp-kubernetes issue

opendatahub-io / notebooks

Notebook images for ODH

Apache License 2.0

17 stars 55 forks source link

Patch elyra kfp template to fix kfp-kubernetes issue #509

Closed harshad16 closed 4 months ago

harshad16 commented 4 months ago

Patch elyra kfp template to fix kfp-kubernetes issue.

This is a temporary fix and would need to be fixed with a better solution.

Description

How Has This Been Tested?

use the PR built tf image.
Execute the fraud-detection pipeline
see the successful pipeline run.

Merge criteria:

[x] The commits are squashed in a cohesive manner and have meaningful messages.
[x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
[x] The developer has manually tested the changes and verified that the changes work

openshift-ci[bot] commented 4 months ago

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

Test name	Commit	Details	Required	Rerun command
ci/prow/notebooks-e2e-tests	4792f3ac136d54e94334ffe3286163cfe9a8dc4e	link	true	`/test notebooks-e2e-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).

atheo89 commented 4 months ago

Shall we add these changes also on tensorflow runtime?

harshad16 commented 4 months ago

Thanls for the work! I am able to get a successful run with kfp==2.5.0 and kfp-kubernetes==1.0.0. I will suggest to remove the definition of the helper methods as well through this patch.

As this a patch, the idea is to remove only the affecting bits. the removal of helper function should be done in permanent fix , directly in elyra once we start working on upgrade of the kfp-kubernetes package in there.

harshad16 commented 4 months ago

Shall we add these changes also on tensorflow runtime?

These changes are not required in tensorflow runtime, as it doesn't contain elyra.

atheo89 commented 4 months ago

Thanks Harshad for this fix! /lgtm

openshift-ci[bot] commented 4 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/opendatahub-io/notebooks/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

harshad16 commented 4 months ago

Thanks for the review merging this .