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

Copy example gen prod #226

Merged
merged 18 commits into from
May 19, 2023
Merged

Conversation

alxndrnh
Copy link
Contributor

@alxndrnh alxndrnh commented Mar 22, 2023

CopyExampleGen component #66:
First pull request for CopyExampleGen component.py file.

  • Tests pass
  • Appropriate changes to README are included in PR

@github-actions
Copy link
Contributor

Thanks for the PR! 🚀

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

@rcrowe-google rcrowe-google removed the request for review from theadactyl March 24, 2023 00:15
@rcrowe-google
Copy link
Collaborator

@rclough @davidxia - Could one of you review this PR? IIRC I think it was originally your use case, right?

tfx_addons/copy_example_gen/README.md Show resolved Hide resolved
tfx_addons/copy_example_gen/README.md Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/README.md Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/README.md Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/README.md Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
@alxndrnh alxndrnh requested review from michaelwsherman and removed request for rcrowe-google March 28, 2023 14:39
Copy link
Contributor

@michaelwsherman michaelwsherman left a comment

Choose a reason for hiding this comment

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

Let me know if the changes to component.py aren't clear. It's important to clean that loop up.

@@ -10,31 +10,73 @@
**Project name:** CopyExampleGen component
Copy link
Contributor

Choose a reason for hiding this comment

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

Add copyright to the readme. Use The Tensorflow Authors (not Google -- see other files in addons).

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 do not see any copyright lines in other readme files. May you link me to an example? @michaelwsherman

tfx_addons/copy_example_gen/__init__.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Outdated Show resolved Hide resolved
tfx_addons/copy_example_gen/component.py Show resolved Hide resolved
Copy link
Contributor

@michaelwsherman michaelwsherman left a comment

Choose a reason for hiding this comment

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

LGTM

@rcrowe-google rcrowe-google self-requested a review May 15, 2023 21:44
@rcrowe-google
Copy link
Collaborator

Could we add some tests?

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.

I would prefer to wait until tests are added, but I think the current authors are off doing other things so it's best to accept this for now and someone can add tests later.

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.

/lgtm
/merge

@rcrowe-google rcrowe-google merged commit 8507648 into tensorflow:main May 19, 2023
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.

3 participants