Closed zyang7 closed 1 year ago
Thanks for the PR! :rocket:
Instructions: Approve using /lgtm
and mark for automatic merge by using /merge
.
/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! :white_check_mark:
PR is approved. Missing merge command to auto-merge PR!
/lgtm /merge
also @zyang7 can you add copy_example_gen
to https://github.com/tensorflow/tfx-addons/blob/main/tfx_addons/version.py#L83 so that tests are run?
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 https://github.com/tensorflow/tfx-addons/pull/251#issuecomment-1603281829, 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! :white_check_mark:
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! :tada:
tfx_addons/copy_example_gen/README.md
for capitalization nits.Checks: