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

docs(kfp): compatibility matrix with TFX #2893

Merged
merged 10 commits into from
Aug 27, 2021
Merged

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Aug 25, 2021

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

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


## Kubeflow Pipelines Backend and TFX compatibility

| Kubeflow Pipelines Backend & [TFX](https://www.tensorflow.org/tfx) Version | <=0.30.0 | 1.0.0 | 1.2.0 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add HTML to DOCSY?

Kubeflow Pipelines Backend [TFX](https://www.tensorflow.org/tfx) Version
<=0.30.0 1.0.0 1.2.0
<=1.6 Compatible Not fully compatible[1] Not fully compatible[1]️
>=1.7 Not fully compatible[1] Not fully compatible[1]️ Compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's possible too. Let think take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to not do this, because

  1. the empty block under "Kubeflow Pipelines Backend" also looks a little ugly
  2. maintainability of html is worse than markdown

I ended up writing the top left cell as "TFX \ KFP Backend", which I think also conveys the information very well with minimal overhead. What do you think?

Copy link
Contributor

@zijianjoy zijianjoy Aug 26, 2021

Choose a reason for hiding this comment

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

Sounds good, agree with the points you have mentioned.

One thing I would like to say is that we probably want to put the TFX to each row along with the version.

So it is like:

[KFP Backend] <= 1.5 >= 1.7
TFX <= 0.28.0 Fully Compatible ✅ Metadata UI not compatible
TFX 0.29.0, 0.30.0 Visualizations not compatible Metadata UI not compatible
TFX 1.0.0 Metadata UI not compatible Metadata UI not compatible
TFX >= 1.2.0 Metadata UI not compatible Fully Compatible ✅

I imagine this matrix to expand in the future, so each row might mean different dependencies, so putting dependency name and version closer to each other helps readability. For example the future will look like :

[KFP Backend] <= 1.5 >= 1.7
TFX <= 0.28.0 Fully Compatible ✅ Metadata UI not compatible
TFX 0.29.0, 0.30.0 Visualizations not compatible Metadata UI not compatible
TFX 1.0.0 Metadata UI not compatible Metadata UI not compatible
TFX >= 1.2.0 Metadata UI not compatible Fully Compatible ✅
TFDA 0.X.X compatible compatible
TFMA 0.X.X compatible compatible
TFMA > 1.0.0 compatible compatible
PyTorch 0.X.X compatible compatible
PyTorch >= 1.0.0 compatible Compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion!
I was planning to add a different table for each dependency, because as you can see there have to be some paragraphs introducing general information about KFP and this dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good on having different tables, thank you Yuan!

@kramachandran
Copy link

I don't think this convey's the information in sufficient depth.

How about this. Let's have the the table be for KFP 1.7

On the Rows list the following features:

  1. Pipeline Execution
  2. TFDV
  3. TFMA
  4. ML Metadata

Then for the columns go with <= 0.30.0, 1.0.0, and 1.2.0

In the cells you can specify for TFDV and TFMA that visualizations don't work

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 26, 2021

@kramachandran Good point for adding more information! I forgot to cover information for tensorflow/tfx#3933.

However, for the specific format, I don't quite want to build one table for one KFP version, because it's harder to find compatible pairs in one entire table. I'll try to use different foot notes to make the information more complete. Let me have another update and you can confirm again.

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 27, 2021

A lot of new comments come from #2895.
Thank you @shuesc and @kramachandran for the feedback! I think I've addressed the major feedback, so I'm going to merge this to prepare for release.
Feel free to continue the discussion and we can adjust.

@Bobgy Bobgy added the lgtm label Aug 27, 2021
@google-oss-robot google-oss-robot merged commit 23a748f into master Aug 27, 2021
@Bobgy Bobgy deleted the tfx-compatibility branch August 27, 2021 04:07
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.

4 participants