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

SDK - Compiler - Stopped adding mlpipeline artifacts to every compiled template #2046

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Sep 5, 2019

Over the life of the Pipelines project we have experienced numerous issues and problems stemming from the auto-added artifacts.
This PR makes a small change to the compiler so that those artifacts are no longer auto-added to every single pipeline step. (All components that actually generate those artifacts continue to work as before.)

Fixes #1421
Fixes #1422
The compiler changes are in the following commit: bdd43a5


This change is Reviewable

Copy link
Member

@elikatsis elikatsis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alexey, thank you for pinging me.

The fact that these two artifacts exist the way they do is problematic and has to be fixed, indeed.
But I'd suggest putting effort on fixing this once and for all. That means:

  • Step 1: Add artifact support for components
  • Step 2: Make this PR change

It will take some time, but hey, it's already been there some time and you have achieved to tackle most probably any problem that has arisen (e.g. #1289).

I can contribute to making this change, but I find myself pretty confused with how components and task factories work (in fact I've been trying to understand them lately). We can cooperate on that if it suits.

sdk/python/kfp/dsl/_container_op.py Show resolved Hide resolved
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 5, 2019

But I'd suggest putting effort on fixing this once and for all. That means:

  • Step 1: Add artifact support for components
  • Step 2: Make this PR change

I have a plan for this, but it's a bit different. I want his PR to go first so that the changes are split into smaller PRs. The plan is to make make all component outputs usable as both parameters and artifacts. The component should not be responsible for deciding how its outputs will be used. Only the consumer decides how to consume the data - via a file or via a command-line argument.
After this is implemented, the special treatment of those two artifacts can be removed and they'll be the same as all other outputs.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 5, 2019

/retest

@Ark-kun Ark-kun force-pushed the SDK---DSL---Stopped-adding-mlpipeline-artifacts-to-every-compiled-template branch from efc9fed to bc48323 Compare September 5, 2019 20:57
@numerology
Copy link

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 5, 2019

/approve

The sample will still work fine as it is now.
I'll add the change to that file as a separate PR.
@k8s-ci-robot k8s-ci-robot added approved and removed lgtm labels Sep 5, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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:

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

@numerology
Copy link

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 6, 2019

/retest

1 similar comment
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 6, 2019

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants