CopyExampleGen tests #251
CopyExampleGen tests #251
Conversation
|
Thanks for the PR! 🚀 Instructions: Approve using |
| 2. Should the dictionary of file inputs take a path to a folder? Globs? Lists of individual files? | ||
| 3. Assuming file copying is done entirely separately, [importer.generate_output_dict](https://github.com/tensorflow/tfx/blob/f8ce19339568ae58519d4eecfdd73078f80f84a2/tfx/dsl/components/common/importer.py#L153) be used as is to register the artifacts, or does some separate code using [MLMD](https://www.tensorflow.org/tfx/guide/mlmd) in a different way need to be written | ||
|
|
||
|
|
||
| ## Project Team | ||
| Alex Ho, alexanderho@google.com, @alxndrnh | ||
|
|
||
| Zi Yang, zya@google.com, @zyang7 |
There was a problem hiding this comment.
README.md file changes lgtm
|
/lgtm Ran component in vertex test with downstream components StatisticsGen and Trainer. Pipeline works and CopyExampleGen() finishes in same amount of time (14 seconds) as original commit. Tests lgtm too. |
|
Approval received from @alxndrnh! ✅ PR is approved. Missing merge command to auto-merge PR! |
|
/lgtm |
|
also @zyang7 can you add Btw we need a merge command by @alxndrnh for this to merge as the sole owner of the component |
|
@casassg Sure I can add it, I'm not sure what dependencies I need, is there a way to validate the versions to dependency mapping is correct? |
|
@zyang7 given the imports in https://github.com/tensorflow/tfx-addons/blob/main/tfx_addons/copy_example_gen/component.py#L45 I think you just need tfx, so you can just add "copy_example_gen": [f"tfx{_TFXVERSION_CONSTRAINT}"]also you can just push here after adding this and it should trigger the tests (it will likely trigger them all given you are changing version.py so it may take a while) but once that runs you can see if its compatible or not. It runs a test w the lowest version of TFX we support and highest one which usually covers pretty good compatibility |
|
Thanks @casassg! Addressed your comments. |
|
We should probably add Zi to the codeowners also.
Robert Crowe | Product Manager, TF OSS & MLOps | ***@***.*** |
@robert_crowe <https://twitter.com/robert_crowe>
…On Thu, Jun 22, 2023 at 1:35 PM Zi ***@***.***> wrote:
Thanks @casassg <https://github.com/casassg>! Addressed your comments.
—
Reply to this email directly, view it on GitHub
<#251 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVWSW67WEPXFYQLIKOODILXMSUATANCNFSM6AAAAAAZNSEADU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
/lgtm /merge Thank you @casassg for the catch and helpful comments. If there's anything else please let us know. Additionally, thank you @zyang7 for the quick turnaround time with these changes. @rcrowe-google I agree we need to add Zi as a codeowner. |
|
Approval and merge request received from @alxndrnh! ✅ PR is missing approvals for some files still. |
|
/lgtm /merge (as version.py owner) |
|
Merged with approvals from alxndrnh and casassg - thanks for the contribution! 🎉 |
tfx_addons/copy_example_gen/README.mdfor capitalization nits.Checks: