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

Use RunInference to perform custom inference in custom_inference notebook #29304

Merged
merged 9 commits into from
Nov 7, 2023

Conversation

AnandInguva
Copy link
Contributor

@AnandInguva AnandInguva commented Nov 4, 2023

Fixes: #29297


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@AnandInguva
Copy link
Contributor Author

R: @damccorm @liferoad

Copy link
Contributor

github-actions bot commented Nov 4, 2023

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@liferoad
Copy link
Contributor

liferoad commented Nov 5, 2023

The changes look good to me. Two suggestions:

  • Can we keep the old way as well? It is a good code snippet for users to learn Beam DoFn
  • Can we link the VertexAIModelHandler notebook inside this?

@damccorm
Copy link
Contributor

damccorm commented Nov 5, 2023

Can we keep the old way as well? It is a good code snippet for users to learn Beam DoFn

I would vote against this. There are other examples of using a DoFn (e.g.

), but the preferred way for people to do remote inference is with RunInference.

@liferoad
Copy link
Contributor

liferoad commented Nov 6, 2023

The current ParDo example is too simple and quite useless. I even think we should not just keep the old way but also use it to show why they should switch to RunInference.

@AnandInguva
Copy link
Contributor Author

AnandInguva commented Nov 6, 2023

The changes look good to me. Two suggestions:

  • Can we keep the old way as well? It is a good code snippet for users to learn Beam DoFn
    I would prefer keeping it simple using RunInference.
  • Can we link the VertexAIModelHandler notebook inside this?
  • there is a notebook on vertex ai and we don't use vertex AI here.

@@ -60,6 +60,7 @@
"To reduce the number of steps that you need to take, RunInference supports features like batching. For more infomation about the RunInference API, review the [RunInference API](https://beam.apache.org/releases/pydoc/current/apache_beam.ml.inference.html#apache_beam.ml.inference.RunInference),\n",
"which demonstrates how to implement model inference in PyTorch, scikit-learn, and TensorFlow.\n",
"\n",
"There is [VertexAIModelHandlerJson](https://github.com/apache/beam/blob/1c24bc94c9ae5cf93dc129e657964858d4aafc67/sdks/python/apache_beam/ml/inference/vertex_ai_inference.py#L61) which is used to make remote inference calls to VertexAI. In this notebook, we will make custom `ModelHandler` to do remote inference calls using CloudVision API.\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we use master or a commit hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified it to master

@AnandInguva AnandInguva merged commit 6c95636 into apache:master Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Move custom_remote_inference.ipynb to use RunInference
3 participants