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

Move MCT component to TFX Addons #196

Merged
merged 76 commits into from
Apr 11, 2023

Conversation

hanneshapke
Copy link
Contributor

@hanneshapke hanneshapke commented Oct 27, 2022

Addresses #82

  • Tests pass
  • Test example notebook
  • Update CODEOWNER

@github-actions
Copy link
Contributor

Thanks for the PR! 🚀

Instructions: Approve using /lgtm and mark for automatic merge by using /merge.

@hanneshapke
Copy link
Contributor Author

@codesue Let's chat async what to do with https://github.com/tensorflow/model-card-toolkit/blob/master/model_card_toolkit/utils/tfx_util.py and the related tests. Are there reasons for it to stay in the MCT repo?

@hanneshapke
Copy link
Contributor Author

I think it should probably stay. It is used in core and it seems to contain the connector pieces to the TFX libraries, not the component par se.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codesue
Copy link
Contributor

codesue commented Oct 27, 2022

Let's chat async what to do with https://github.com/tensorflow/model-card-toolkit/blob/master/model_card_toolkit/utils/tfx_util.py and the related tests.

This might be good to consider as part of tensorflow/model-card-toolkit#228. I wonder if it's feasible to move functionality specific to pipelines and mlmd to the component's new home and the mlmd client library respectively but keep functions that only used tfma, tfdv, etc. If so, we could make tfx and mlmd utils available through a [tfx] extra, for example.

rcrowe-google and others added 11 commits April 2, 2023 19:06
First draft of project proposal.
Oops, forgot to add the dependencies!
Adding a note about statistics and schema for the changed dataset.
* Create 20220513-pandas_transform.md

* Delete 20220513-PandasTransform.md
Fixing broken links on PyPI page.
* Cherry pick bot to backport changes

* Update RELEASE.md

* validate user is release manager
* Updated module file for compatibility

* Updated module file for compatibility

* Update iris_module_file.py

* Created Palmer Penguins example using Colaboratory

* Created Pima Indians Diabetes example pipeline using Colaboratory

* Created Iris example pipeline using Colaboratory

* Deleted outdated file + replaced with new examples

* Deleted outdated file
Commit 93fd006 didn't correctly fix the link.
* include TFX 1.8

* increase CI

* switch cache key to constaint first

* remove shared cache

* fix exit handler

* fix patch issues

* reformating yapf
@hanneshapke hanneshapke marked this pull request as ready for review April 4, 2023 17:35
Copy link
Collaborator

@rcrowe-google rcrowe-google left a comment

Choose a reason for hiding this comment

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

Thanks!
/lgtm

Copy link
Contributor

@codesue codesue left a comment

Choose a reason for hiding this comment

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

lgtm after tests pass

CODEOWNERS Outdated
@@ -61,6 +61,10 @@
# PandasTransform Component
/tfx_addons/pandas_transform @rcrowe-google

# PandasTransform Component
/tfx_addons/model_card_generator @codesue @hanneshapke
/eamples/model_card_generator @codesue @hanneshapke
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arg, good catch.

.gitignore Outdated
.vscode/*

# unnecessary project files
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving this to examples/model_card_generator/.gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Let's revert the changes to this file before merging.

.gitignore Outdated
.vscode/*

# unnecessary project files
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Let's revert the changes to this file before merging.

@codesue
Copy link
Contributor

codesue commented Apr 11, 2023

/lgtm

@github-actions
Copy link
Contributor

Approval received from @codesue! ✅

PR is approved. Missing merge command to auto-merge PR!

@hanneshapke
Copy link
Contributor Author

Thank you @codesue

@hanneshapke
Copy link
Contributor Author

/merge

@github-actions github-actions bot merged commit 5b4d197 into tensorflow:main Apr 11, 2023
@github-actions
Copy link
Contributor

Merged with approvals from codesue and rcrowe-google - thanks for the contribution! 🎉

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.

None yet

7 participants