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

Components - TFX #2671

Merged
merged 26 commits into from
Dec 5, 2019
Merged

Components - TFX #2671

merged 26 commits into from
Dec 5, 2019

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Nov 27, 2019

Added the KFP-native TFX components and a notebook with a sample pipeline.

The components have no custom code - they just import tfx package and use the components and executors.


This change is Reviewable

Copy link

@numerology numerology left a comment

Choose a reason for hiding this comment

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

Nice. Will continue reviewing shortly after.

components/tfx/Evaluator/component.py Outdated Show resolved Hide resolved
def Evaluator(
examples_path: InputPath('Examples'),
model_exports_path: InputPath('Model'),
#model_path: InputPath('Model'),

Choose a reason for hiding this comment

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

Can we use model_path from the very beginning. I think TFX is migrating to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit non-trivial since the ComponentSpec.INPUTS uses the old names.
I'll solve this issue in another PR.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Dec 2, 2019

@numerology The tests are passing now.
/approve

@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

@neuromage
Copy link
Contributor

@Ark-kun what use-case do these components enable that cannot already be achieved by directly using the TFX components and DSL as is?

@numerology
Copy link

Maybe a similar question to @neuromage 's: do we have any plan regarding how these TFX components will be maintained alongside the evolution of uDSL?

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Dec 2, 2019

any plan regarding how these TFX components will be maintained

These components do not need much maintenance as they just directly use the TFX package.
We'll just need to bump the image version when TFX OSS releases a new version.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Dec 2, 2019

@Ark-kun what use-case do these components enable that cannot already be achieved by directly using the TFX components and DSL as is?

These components enable the customers of Kubeflow Pipelines to use the Tensorflow Extended components.
Today, the KFP customers can use XGBoost, SKLearn and other other ML toolkits.
It makes sense to help the customers use Tensorflow Extended as well.
This also enables KF Pipelines' customers to compose pipelines that combine KFP components and TFX components - an often requested feature.

@neuromage
Copy link
Contributor

@Ark-kun they can already run TFX components on KFP using KubeflowDagRunner, which I believe is the supported path going forward.

Copy link
Contributor

@animeshsingh animeshsingh left a comment

Choose a reason for hiding this comment

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

@neuromage @Ark-kun i think having both the modes help? For folks in KF ecosystem, they can leverage the KFP DSL, whereas those in TFX can leverage the TF DSL?

@numerology
Copy link

@neuromage @Ark-kun i think having both the modes help? For folks in KF ecosystem, they can leverage the KFP DSL, whereas those in TFX can leverage the TF DSL?

One minor clarification: Current KFP DSL and SDK will not be deprecated. It'll still be supported going forward.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Dec 2, 2019

@neuromage @Ark-kun i think having both the modes help? For folks in KF ecosystem, they can leverage the KFP DSL, whereas those in TFX can leverage the TF DSL?

I think this is a good idea.
Otherwise it might confuse our customers if we support XGBoost, SKLearn and PyTorch while artificially excluding Tensorflow from the components.

Let's merge this PR and move on to work on improving the DSL usability.

@animeshsingh
Copy link
Contributor

/lgtm

@animeshsingh
Copy link
Contributor

@Ark-kun @neuromage any chance we can present this to MLOps sig meeting tomorrow (at 9 PST) - essentially put both the DSLs side by side and discuss?

@k8s-ci-robot k8s-ci-robot merged commit b634720 into kubeflow:master Dec 5, 2019
@neuromage
Copy link
Contributor

These components seem to wrap the existing TFX components, but don't make use of the driver/publisher bits, which means we don't get metadata tracking. They are also pinned to a specific version of TFX, and makes assumptions about their internal implementation behaviour. Hence, this pattern will not scale in terms of maintainability of these components. I'm also not sure what use-case these components support right now that is not already enabled by the TFX DSL?

/cc @paveldournov
/cc @jessiezcc

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

Successfully merging this pull request may close these issues.

5 participants