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

Feature/pred2bq executor refactor #222

Merged

Conversation

cfezequiel
Copy link
Contributor

Fixes #221

  • Refactors executor.py

  • Adds unit tests

  • Adds credits to original code author

  • [ X ] Tests pass

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Thanks for the PR! 🚀

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

- Adds unit tests
- Also adds credits to original code author
@cfezequiel cfezequiel force-pushed the feature/pred2bq-executor-refactor branch from b200795 to 8a477fc Compare March 2, 2023 15:02
data Outdated Show resolved Hide resolved
@rcrowe-google rcrowe-google removed the request for review from theadactyl March 2, 2023 18:38
@cfezequiel
Copy link
Contributor Author

@hanneshapke @rcrowe-google if the PR looks good I was wondering if we could already merge it. I'd like to raise a subsequent PR for further code changes to the component.

@hanneshapke
Copy link
Contributor

@cfezequiel I am sorry, I won't have a chance to review the PR in detail. A brief review looked good, but the PR is a bigger rewrite than what I can handle after work at the moment. I am sorry.

@hanneshapke
Copy link
Contributor

hanneshapke commented Mar 14, 2023

@cfezequiel Just did another pass over the PR. Could you please remove my email address from the files. Can you leave my name and add my affiliation please (e.g. Hannes Hapke (Digits Financial Inc.))? Thank you.

Happy to stamp / merge afterward.

@cfezequiel
Copy link
Contributor Author

@cfezequiel Just did another pass over the PR. Could you please remove my email address from the files. Can you leave my name and add my affiliation please (e.g. Hannes Hapke (Digits Financial Inc.))? Thank you.

Happy to stamp / merge afterward.

@hanneshapke Sure, done.

@hanneshapke
Copy link
Contributor

/lgtm

@hanneshapke
Copy link
Contributor

@cfezequiel Thank you.

@github-actions
Copy link
Contributor

Approval received from @hanneshapke! ✅

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

@hanneshapke
Copy link
Contributor

/merge

@github-actions github-actions bot merged commit 405495d into tensorflow:main Mar 14, 2023
@github-actions
Copy link
Contributor

Merged with approvals from hanneshapke - thanks for the contribution! 🎉

@cfezequiel
Copy link
Contributor Author

Thanks for the approval and merge @hanneshapke .

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.

_parse_example error when input contains SparseTensor in predictions-to-bigquery component.
2 participants