-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Components - TFX #2671
Conversation
… original component
First draft
First draft
Input artifacts must have splits. Split URIs should end with "/'. The ciomponents now work. Also printing component_class_instance for debugging.
The component uses almost completely generic code.
Added ExampleValidator, Transform, Trainer, Evaluator
There was a problem hiding this 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.
def Evaluator( | ||
examples_path: InputPath('Examples'), | ||
model_exports_path: InputPath('Model'), | ||
#model_path: InputPath('Model'), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
edf9d8f
to
807e730
Compare
807e730
to
bbf11e3
Compare
@numerology The tests are passing now. |
[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 |
@Ark-kun what use-case do these components enable that cannot already be achieved by directly using the TFX components and DSL as is? |
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? |
These components do not need much maintenance as they just directly use the TFX package. |
These components enable the customers of Kubeflow Pipelines to use the Tensorflow Extended components. |
@Ark-kun they can already run TFX components on KFP using KubeflowDagRunner, which I believe is the supported path going forward. |
There was a problem hiding this 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?
One minor clarification: Current KFP DSL and SDK will not be deprecated. It'll still be supported going forward. |
I think this is a good idea. Let's merge this PR and move on to work on improving the DSL usability. |
/lgtm |
@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? |
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 |
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