You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/06/13 13:22:39 UTC
[GitHub] [beam] AnandInguva opened a new pull request, #21819: Refactor code according to keyedModelHandler changes
AnandInguva opened a new pull request, #21819:
URL: https://github.com/apache/beam/pull/21819
Resolve tests failing due to #21777.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21819:
URL: https://github.com/apache/beam/pull/21819#issuecomment-1153963609
> @AnandInguva can you fix the input and state_dict path names too? Or maybe in a separate PR
will look into it in a separate PR. thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895723812
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
+ # PytorchModelHandler on KeyedModelHandler for keyed examples.
+ model_loader = KeyedModelHandler(
Review Comment:
nit: model_handler?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895721717
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
+ # PytorchModelHandler on KeyedModelHandler for keyed examples.
+ model_loader = KeyedModelHandler(
+ PytorchModelHandler(
+ state_dict_path=known_args.model_state_dict_path,
+ model_class=model_class,
+ model_params=model_params))
Review Comment:
yes, still getting the error(expecting Union) if I remove the output hint.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895723191
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
Review Comment:
This sounds like the input is always keyed. Isn't it user's choice? If so, would this be appropriate instead:
```
In this example we pass keyed inputs to RunInference transform.
Therefore, we use KeyedModelHandler wrapper over PytorchModelHandler.
```
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
Review Comment:
This sounds like the input is always keyed. Isn't it user's choice? If so, would this be more appropriate instead:
```
In this example we pass keyed inputs to RunInference transform.
Therefore, we use KeyedModelHandler wrapper over PytorchModelHandler.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895733289
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
+ # PytorchModelHandler on KeyedModelHandler for keyed examples.
+ model_loader = KeyedModelHandler(
+ PytorchModelHandler(
+ state_dict_path=known_args.model_state_dict_path,
+ model_class=model_class,
+ model_params=model_params))
Review Comment:
Do you think it's because `RunInference` is imported from `api.py` instead of `base.py` and the `RunInference` in `api.py` has `Union[...]` type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] yeandy commented on pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
yeandy commented on PR #21819:
URL: https://github.com/apache/beam/pull/21819#issuecomment-1153944796
@AnandInguva can you fix the input and state_dict path names too? Or maybe in a separate PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn merged pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
tvalentyn merged PR #21819:
URL: https://github.com/apache/beam/pull/21819
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21819:
URL: https://github.com/apache/beam/pull/21819#issuecomment-1153909862
R: @robertwb @tvalentyn @yeandy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21819:
URL: https://github.com/apache/beam/pull/21819#issuecomment-1153910992
Run Python 3.7 PostCommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895734980
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
+ # PytorchModelHandler on KeyedModelHandler for keyed examples.
+ model_loader = KeyedModelHandler(
+ PytorchModelHandler(
+ state_dict_path=known_args.model_state_dict_path,
+ model_class=model_class,
+ model_params=model_params))
Review Comment:
ok. I thought the splitting of model handlers would allow us getting the type deterministically instead of having to make a union. we can look into it separately.
cc: @robertwb @yeandy @rezarokni
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895738776
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895713829
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
+ # PytorchModelHandler on KeyedModelHandler for keyed examples.
+ model_loader = KeyedModelHandler(
+ PytorchModelHandler(
+ state_dict_path=known_args.model_state_dict_path,
+ model_class=model_class,
+ model_params=model_params))
Review Comment:
Do we still need the type hints on line 139?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21819:
URL: https://github.com/apache/beam/pull/21819#issuecomment-1153962557
Ran the IT test that uses the above example offline and it passes. Also all the unit test for torch RunInference tests passes locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tvalentyn commented on a diff in pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
tvalentyn commented on code in PR #21819:
URL: https://github.com/apache/beam/pull/21819#discussion_r895713829
##########
sdks/python/apache_beam/examples/inference/pytorch_image_classification.py:
##########
@@ -114,10 +115,13 @@ def run(argv=None, model_class=None, model_params=None, save_main_session=True):
model_class = MobileNetV2
model_params = {'num_classes': 1000}
- model_loader = PytorchModelLoader(
- state_dict_path=known_args.model_state_dict_path,
- model_class=model_class,
- model_params=model_params)
+ # the input to RunInference transform is keyed. Wrap
+ # PytorchModelHandler on KeyedModelHandler for keyed examples.
+ model_loader = KeyedModelHandler(
+ PytorchModelHandler(
+ state_dict_path=known_args.model_state_dict_path,
+ model_class=model_class,
+ model_params=model_params))
Review Comment:
Do we still need the type hints on line 138?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] AnandInguva commented on pull request #21819: Refactor code according to keyedModelHandler changes
Posted by GitBox <gi...@apache.org>.
AnandInguva commented on PR #21819:
URL: https://github.com/apache/beam/pull/21819#issuecomment-1153911171
Run Python 3.9 PostCommit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org