Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spike] Discussion on how to trim down Tekton/Argo go pkg to resolve go.mod conflicts. #329

Closed
HumairAK opened this issue Sep 14, 2023 · 7 comments
Labels
priority/normal An issue with the product; fix when possible triage/accepted v2-integration

Comments

@HumairAK
Copy link
Contributor

Review the pain points associated with conflicting tekton/argo packages:

Original ticket: kubeflow/kfp-tekton#1240
TektonCD issue: tektoncd/pipeline#6483

Investigate where we can help in pushing this forward as it's identified as a major blocker for getting kfp-v2 merged into upstream kfp.

Acceptance criteria:

  • Doc thoroughly explaining current issues associated with conflicting packages
  • Outline clear/concise actionable next steps as GH issues
@HumairAK HumairAK added triage/accepted priority/normal An issue with the product; fix when possible v2-integration labels Sep 14, 2023
@Tomcli
Copy link

Tomcli commented Sep 25, 2023

Hey I just saw this issue right now.

From a technical level, we are able to run both Argo and Tekton using the same go.mod. However, the following list of packages are in a high risk of future dependency conflicts.

  • go client packages : Issue from Knaitve such as Third party libraries are not latest version knative/pkg#2759 could delay Tekton from migrating to the latest go client version which in terms could block Kubeflow upstream go client version upgrade. Same could happen the other way around where Knative may require a much newer version of go client API which some of the KFP dependencies may have. This is actually the main blocker when we first integrated Tekton into KFP where Tekton and Argo cannot use the same go client version. At the time we implemented all the necessary features to Tekton upstream, so we cannot use an older Tekton client which forced us to create KFP-Tekton for our initial POC.
  • klog vs glog: When we are creating any custom task controller to optimize the performance of KFP (such as running the KFP driver as custom task), we see conflicts with the klog package because Tekton custom tasks are built with Knative controller and KFP packages are built with glog. Currently we have to patch a diff file when creating the Knative vendor during the container build time in order to resolve this conflict. https://github.com/kubeflow/kfp-tekton/blob/v2-integration/backend/src/v2/controller/klog.patch

For all the custom task features (such as loop with parallelism, sub-dag, looping on thousand plus iterations, KFP driver/publisher optimization), we would need to use the Knative package because we need to run the Knative controller code from Tekton in this case. So these packages should stay with KFP-Tekton because custom tasks can be built from importing golang package instead of syncing upstream.

As for the Tekton golang compiler/executor engine, KFP only needs the Tekton APIs and informers to create and watch Tekton Pipelineruns. Therefore, if we can remove the Knative dependency from the Tekton API package, KFP can reduce the risk of Tekton using a incompatible golang version. Another benefit is KFP can reduce the list of third party dependencies which can lower the image size and potential incapable licenses.

@HumairAK
Copy link
Contributor Author

Thank you @Tomcli I will go over this. This is very helpful.

Therefore, if we can remove the Knative dependency from the Tekton API package, KFP can reduce the risk of Tekton using a incompatible golang version.

So this sounds like the crux of what remains and needs to be done yes? The rest of the knative dependency collisions happen in all the custom task controllers' logic right? Which you are proposing to keep in kfp-tekton repo and import as packages. Am I understanding this correctly?

@Tomcli
Copy link

Tomcli commented Sep 26, 2023

Thank you @Tomcli I will go over this. This is very helpful.

Therefore, if we can remove the Knative dependency from the Tekton API package, KFP can reduce the risk of Tekton using a incompatible golang version.

So this sounds like the crux of what remains and needs to be done yes? The rest of the knative dependency collisions happen in all the custom task controllers' logic right? Which you are proposing to keep in kfp-tekton repo and import as packages. Am I understanding this correctly?

I think we only want to push the Tekton compiler code upstream (under /backend/v2/compiler and related common packages). All the custom task controllers code for KFP-Tekton can stay in KFP-Tekton because they are mostly independent images.

@Tomcli
Copy link

Tomcli commented Sep 26, 2023

Therefore, if we can remove the Knative dependency from the Tekton API package, KFP can reduce the risk of Tekton using a incompatible golang version.

After some investigation, removing the Knative dependency would be pretty difficult for current Tekton implementation because Tekton Pipelinerun.status struct is from Knative. This is definitely a nice to have as I see the Knative package is more of a risk than benefit because the contributions in the Knative community is going down.

@HumairAK
Copy link
Contributor Author

Okay thanks @Tomcli that's good to know. I'll look into seeing if we can inquire with the tekton devs about this.

I see the Knative package is more of a risk than benefit because the contributions in the Knative community is going down.

Given that most of the v2 tekton custom task controllers leverage the knative controller frameworks -- do you fore see this being another point of concern?

@Tomcli
Copy link

Tomcli commented Sep 27, 2023

Given that most of the v2 tekton custom task controllers leverage the knative controller frameworks -- do you fore see this being another point of concern?

From the controller perspective, since the Tekton core controller is using Knative, I don't think we can mitigate the risk even if we change all our custom task controllers to not use Knative. Since custom task controllers are only built for Tekton, we are less likely to have package conflicts when we import the upstream code for optimizing the driver/publisher features.

The risk I see in Knative is more on its data struct. Since major part of the Tekton Status inherited from the Knative package, when we want to have KFP V2 supporting both Argo and Tekton data struct on the same server, any major conflict on the Kubernetes/go-client spec could break the compatibility.

@gregsheremeta
Copy link
Contributor

Migrated to Jira: https://issues.redhat.com/browse/RHOAIENG-1661

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/normal An issue with the product; fix when possible triage/accepted v2-integration
Development

No branches or pull requests

3 participants