Split PytorchModelHandler into PytorchModelHandlerTensor and PytorchModelHandlerKeyedTensor #21810

yeandy · 2022-06-10T21:12:47Z

Split PytorchModelHandler into experimental PytorchModelHandlerTensor and PytorchModelHandlerKeyedTensor transforms.

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

Choose reviewer(s) and mention them in a comment (R: @username).
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)

See CI.md for more information about GitHub Actions CI.

yeandy · 2022-06-10T21:16:36Z

sdks/python/apache_beam/ml/inference/pytorch_inference.py

+from apache_beam.utils.annotations import experimental


 class PytorchModelHandler(ModelHandler[torch.Tensor,


Is the intention to still keep PytorchModelHandler?

No I think we want to drop PytorchModelHandler

yeandy · 2022-06-10T21:16:46Z

sdks/python/apache_beam/ml/inference/pytorch_inference_test.py

  import torch
  from apache_beam.ml.inference.api import PredictionResult
  from apache_beam.ml.inference.base import RunInference
  from apache_beam.ml.inference.pytorch_inference import PytorchModelHandler


For the tests, I changed them to use PytorchModelHandlerTensor, and PytorchModelHandlerKeyedTensor. Should the tests default to use PytorchModelHandler since PytorchModelHandlerTensor, and PytorchModelHandlerKeyedTensor are experimental? How should we handle this

Let's drop PytorchModelHandler and exercise the type-specific ones.

yeandy · 2022-06-10T21:17:15Z

R: @TheNeuralBit @ryanthompson591 @robertwb

asf-ci · 2022-06-10T21:24:20Z

Can one of the admins verify this patch?

asf-ci · 2022-06-10T21:24:20Z

Can one of the admins verify this patch?

asf-ci · 2022-06-10T21:24:20Z

Can one of the admins verify this patch?

asf-ci · 2022-06-10T21:24:20Z

Can one of the admins verify this patch?

TheNeuralBit

Thanks!

sdks/python/apache_beam/ml/inference/pytorch_inference.py

TheNeuralBit · 2022-06-10T22:43:19Z

sdks/python/apache_beam/ml/inference/pytorch_inference.py

+  """ Implementation of the ModelHandler interface for PyTorch.
+
+      NOTE: This API and its implementation are under development and
+      do not provide backward compatibility guarantees.


IIUC we want to drop the experimental tag and NOTE for this one, but we'll keep the named input one (KeyedTensor) marked experimental. See here: #21803 (comment)

TheNeuralBit · 2022-06-10T22:45:03Z

sdks/python/apache_beam/ml/inference/pytorch_inference.py

+from apache_beam.utils.annotations import experimental


 class PytorchModelHandler(ModelHandler[torch.Tensor,


No I think we want to drop PytorchModelHandler

TheNeuralBit · 2022-06-10T22:46:02Z

sdks/python/apache_beam/ml/inference/pytorch_inference_test.py

  import torch
  from apache_beam.ml.inference.api import PredictionResult
  from apache_beam.ml.inference.base import RunInference
  from apache_beam.ml.inference.pytorch_inference import PytorchModelHandler


Let's drop PytorchModelHandler and exercise the type-specific ones.

yeandy · 2022-06-13T18:07:58Z

PTAL @TheNeuralBit

codecov · 2022-06-13T19:10:23Z

Codecov Report

Merging #21810 (aa6d026) into master (383c08d) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #21810      +/-   ##
==========================================
- Coverage   74.15%   74.13%   -0.03%     
==========================================
  Files         698      698              
  Lines       92417    92429      +12     
==========================================
- Hits        68530    68519      -11     
- Misses      22636    22659      +23     
  Partials     1251     1251

Flag	Coverage Δ
python	`83.74% <0.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...thon/apache_beam/ml/inference/pytorch_inference.py	`0.00% <0.00%> (ø)`
...n/apache_beam/ml/gcp/recommendations_ai_test_it.py	`73.46% <0.00%> (-2.05%)`	⬇️
.../python/apache_beam/transforms/periodicsequence.py	`96.72% <0.00%> (-1.64%)`	⬇️
sdks/python/apache_beam/io/source_test_utils.py	`88.01% <0.00%> (-1.39%)`	⬇️
...eam/runners/portability/fn_api_runner/execution.py	`92.44% <0.00%> (-0.65%)`	⬇️
...ks/python/apache_beam/runners/worker/sdk_worker.py	`88.94% <0.00%> (-0.16%)`	⬇️
...examples/inference/pytorch_image_classification.py	`0.00% <0.00%> (ø)`
sdks/python/apache_beam/ml/inference/api.py
sdks/python/apache_beam/ml/inference/__init__.py	`100.00% <0.00%> (ø)`
sdks/python/apache_beam/io/gcp/pubsub.py	`90.78% <0.00%> (+0.13%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 383c08d...aa6d026. Read the comment docs.

ryanthompson591 · 2022-06-14T14:08:21Z

sdks/python/apache_beam/ml/inference/pytorch_inference.py

+        self._model_class,
+        self._state_dict_path,
+        self._device,
+        **self._model_params)


What do you think about naming model parameters as a dictionary?

The advantage is that users can specify exactly what their parameters should be.

They would specify the parameters like this:

model_parameters = {
'key_1': 'parameter_1'
}

Then in the future if optional parameters are added they won't collide.

Feel free to do that change in another PR if you think it's a good idea.

are you referring to something like this? #21806

…odelHandlerKeyedTensor (apache#21810) * Start to split of Pytorch handlers * Remove old PytorchModelHandler

Start to split of Pytorch handlers

27dbd4f

github-actions bot added the python label Jun 10, 2022

yeandy commented Jun 10, 2022

View reviewed changes

TheNeuralBit reviewed Jun 10, 2022

View reviewed changes

yeandy added 3 commits June 13, 2022 07:41

Remove old PytorchModelHandler

718669e

Merge master

cb23895

Merge branch 'master'

e632ad4

Merge master

aa6d026

ryanthompson591 approved these changes Jun 14, 2022

View reviewed changes

TheNeuralBit merged commit a94feb1 into apache:master Jun 14, 2022

tvalentyn mentioned this pull request Jun 15, 2022

[Task]: Add input type hint to inference model loader #21769

Closed

bullet03 pushed a commit to akvelon/beam that referenced this pull request Jun 20, 2022

Split PytorchModelHandler into PytorchModelHandlerTensor and PytorchM…

3575799

…odelHandlerKeyedTensor (apache#21810) * Start to split of Pytorch handlers * Remove old PytorchModelHandler

		from apache_beam.utils.annotations import experimental


		class PytorchModelHandler(ModelHandler[torch.Tensor,

Split PytorchModelHandler into PytorchModelHandlerTensor and PytorchModelHandlerKeyedTensor #21810

Split PytorchModelHandler into PytorchModelHandlerTensor and PytorchModelHandlerKeyedTensor #21810

Uh oh!

Conversation

yeandy commented Jun 10, 2022

GitHub Actions Tests Status (on master branch)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeandy commented Jun 10, 2022

Uh oh!

asf-ci commented Jun 10, 2022

Uh oh!

asf-ci commented Jun 10, 2022

Uh oh!

asf-ci commented Jun 10, 2022

Uh oh!

asf-ci commented Jun 10, 2022

Uh oh!

TheNeuralBit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeandy commented Jun 13, 2022

Uh oh!

codecov bot commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 13, 2022 •

edited

Loading