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

Prediction to bigquery component - initial code #210

Merged
merged 5 commits into from
Feb 6, 2023

Conversation

hanneshapke
Copy link
Contributor

@hanneshapke hanneshapke commented Feb 5, 2023

Related to issue #78

This is the OSS version of Digit's predictions to bigquery component.

What's missing and not part of this PR?

  • dynamic BQ schema read from prediction_log
  • tests covering the dynamic BQ schema

The component used by Digits' TFX pipelines.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

Thanks for the PR! 🚀

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

@michaelwsherman
Copy link
Contributor

LGTM

@hanneshapke hanneshapke merged commit 4862345 into tensorflow:main Feb 6, 2023
@casassg
Copy link
Member

casassg commented Feb 6, 2023

@hanneshapke seems the linter is failing mind fixing it up? Else new PRs may be blocked from merging https://github.com/tensorflow/tfx-addons/actions/runs/4099019498/jobs/7068535397

@casassg
Copy link
Member

casassg commented Feb 6, 2023

(also let's try to stick to using the automerge bot as bot ensures all tests/linters pass before merging avoiding issues like this)

@@ -0,0 +1,100 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's add the Copyright headers here please.

@hanneshapke
Copy link
Contributor Author

@casassg Sorry, I forgot to leave a comment about it. @michaelwsherman wanted to take over the project urgently. The component is missing the last piece (dyn schema) and it is therefore not functional. We shared it via the project, because @michaelwsherman can't fork from my dev branch.

@casassg
Copy link
Member

casassg commented Feb 6, 2023

@hanneshapke or @michaelwsherman mind adding lint ignore to all files until those get fixed? Want to avoid having to deal w issues on new PRs ideally

@michaelwsherman
Copy link
Contributor

michaelwsherman commented Feb 6, 2023

Echoing @hanneshapke, Google is going to finish up getting this merged in properly and adding some tests. But it may be a week or two before that's all done.

@casassg -- sorry about the rough edges here, but there was some challenges figuring out a way for Google to work on Hannes code, and getting the working code into the tfx-addons repo was the best way we could figure out how to collaborate. Would it be safer to move this "working" code into a subfolder of the repo root rather a subfolder of tfx_addons where it's in the package?

@casassg
Copy link
Member

casassg commented Feb 6, 2023

It's fine to merge, the issue is linter runs on all python files. Just adding linter-skip annotations to all new files is enough. I can do it if needed

@hanneshapke
Copy link
Contributor Author

Happy to craete a fix for it in a moment

@michaelwsherman
Copy link
Contributor

michaelwsherman commented Feb 6, 2023

@cfezequiel to add linter-skip annotations if Hannes doesn't get to it first. Sorry folks, on the move today and hard for me to just bang this out myself.

@casassg
Copy link
Member

casassg commented Feb 7, 2023

Got it fixed up, should be fine now. Let's try to remove the linter skip annotations

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