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/05/26 23:42:08 UTC

[GitHub] [beam] ihji opened a new pull request, #17773: [BEAM-14524] Add return_row option to RunInference transform

ihji opened a new pull request, #17773:
URL: https://github.com/apache/beam/pull/17773

   Change the interface of RunInference transform for better multi-language compatibility.
   - added return_row option for returning language-neutral row type object instead of Python-specific PredictionResult object
   - remove output type hints which prevents other SDKs from manually overriding output types of RunInference external transform
   
   ------------------------
   
   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`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] 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)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](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] ryanthompson591 commented on pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on PR #17773:
URL: https://github.com/apache/beam/pull/17773#issuecomment-1152422485

   cc: @yeandy @AnandInguva 


-- 
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] ihji commented on a diff in pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ihji commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r891785019


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -52,8 +52,12 @@ class RunInference(beam.PTransform):
 
   TODO(BEAM-14046): Add and link to help documentation
   """
-  def __init__(self, model_loader: base.ModelLoader):
-    self._model_loader = model_loader
+  def __init__(
+      self, model_loader: Union[base.ModelLoader, Callable], return_row=False):

Review Comment:
   Changed PredictionResult from a data class to a named tuple.



-- 
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] ihji commented on pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ihji commented on PR #17773:
URL: https://github.com/apache/beam/pull/17773#issuecomment-1155956862

   Run Python PreCommit


-- 
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] ihji commented on a diff in pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ihji commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r891788348


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -52,8 +52,12 @@ class RunInference(beam.PTransform):
 
   TODO(BEAM-14046): Add and link to help documentation
   """
-  def __init__(self, model_loader: base.ModelLoader):
-    self._model_loader = model_loader
+  def __init__(
+      self, model_loader: Union[base.ModelLoader, Callable], return_row=False):
+    self._model_loader = model_loader() if isinstance(

Review Comment:
   Adding `create` method so that we could separate the class constructor and the arguments. The low-level API from the Java SDK would look like:
   ```
   PythonExternalTransform.from("apache_beam.ml.inference.api.RunInference.create")
       .withKwarg("model_loader_provider", PythonCallableSource.of(
           "from apache_beam.ml.inference.sklearn_inference import SklearnModelLoader\nSklearnModelLoader"))
       .withKwarg("model_uri", "/tmp/staged/sklearn_model")
   ```



-- 
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] ihji commented on pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ihji commented on PR #17773:
URL: https://github.com/apache/beam/pull/17773#issuecomment-1155887762

   Run Python PreCommit


-- 
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] codecov[bot] commented on pull request #17773: [BEAM-14524] Add return_row option to RunInference transform

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17773:
URL: https://github.com/apache/beam/pull/17773#issuecomment-1139160465

   # [Codecov](https://codecov.io/gh/apache/beam/pull/17773?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#17773](https://codecov.io/gh/apache/beam/pull/17773?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (059f3f0) into [master](https://codecov.io/gh/apache/beam/commit/92b8dc75286268f85da59d436ef50e5913dbe9e5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (92b8dc7) will **decrease** coverage by `0.00%`.
   > The diff coverage is `82.35%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #17773      +/-   ##
   ==========================================
   - Coverage   74.00%   74.00%   -0.01%     
   ==========================================
     Files         695      695              
     Lines       91866    91870       +4     
   ==========================================
   - Hits        67989    67988       -1     
   - Misses      22631    22636       +5     
     Partials     1246     1246              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | python | `83.74% <82.35%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/17773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [sdks/python/apache\_beam/ml/inference/api.py](https://codecov.io/gh/apache/beam/pull/17773/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvaW5mZXJlbmNlL2FwaS5weQ==) | `85.71% <72.72%> (-4.29%)` | :arrow_down: |
   | [sdks/python/apache\_beam/ml/inference/base.py](https://codecov.io/gh/apache/beam/pull/17773/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvaW5mZXJlbmNlL2Jhc2UucHk=) | `92.85% <100.00%> (+0.15%)` | :arrow_up: |
   | [sdks/python/apache\_beam/internal/gcp/auth.py](https://codecov.io/gh/apache/beam/pull/17773/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvZ2NwL2F1dGgucHk=) | `73.33% <0.00%> (-5.34%)` | :arrow_down: |
   | [...n/apache\_beam/ml/gcp/recommendations\_ai\_test\_it.py](https://codecov.io/gh/apache/beam/pull/17773/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvZ2NwL3JlY29tbWVuZGF0aW9uc19haV90ZXN0X2l0LnB5) | `73.46% <0.00%> (-2.05%)` | :arrow_down: |
   | [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/17773/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.67% <0.00%> (+0.12%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17773?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17773?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [92b8dc7...059f3f0](https://codecov.io/gh/apache/beam/pull/17773?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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 #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
tvalentyn merged PR #17773:
URL: https://github.com/apache/beam/pull/17773


-- 
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] ihji commented on a diff in pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ihji commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r891788571


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -17,16 +17,17 @@
 # mypy: ignore-errors
 
 from dataclasses import dataclass
-from typing import Tuple
-from typing import TypeVar
-from typing import Union
+from typing import Callable
 
 import apache_beam as beam
 from apache_beam.ml.inference import base
+from apache_beam.typehints import Tuple

Review Comment:
   Removed.



-- 
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] robertwb commented on a diff in pull request #17773: [BEAM-14524] Add return_row option to RunInference transform

Posted by GitBox <gi...@apache.org>.
robertwb commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r891720599


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -52,8 +52,12 @@ class RunInference(beam.PTransform):
 
   TODO(BEAM-14046): Add and link to help documentation
   """
-  def __init__(self, model_loader: base.ModelLoader):
-    self._model_loader = model_loader
+  def __init__(
+      self, model_loader: Union[base.ModelLoader, Callable], return_row=False):
+    self._model_loader = model_loader() if isinstance(

Review Comment:
   I think this is a case where we'll want to provide a wrapping transform rather than do a type check (which would also force model loaders to never implement callable). Mostly that is because I think we well almost always want to provide arguments to the model loader, and being able to pass `PythonCallable('path.to.ModelType', arg0, arg1, ...)` will be more convenient (and less error prone) than trying to inline the arguments in the PythonCallable string itself. 



-- 
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 pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
tvalentyn commented on PR #17773:
URL: https://github.com/apache/beam/pull/17773#issuecomment-1155315905

   What are the next steps on this 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] ryanthompson591 commented on a diff in pull request #17773: [BEAM-14524] Add return_row option to RunInference transform

Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r883172356


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -52,8 +52,12 @@ class RunInference(beam.PTransform):
 
   TODO(BEAM-14046): Add and link to help documentation
   """
-  def __init__(self, model_loader: base.ModelLoader):
-    self._model_loader = model_loader
+  def __init__(
+      self, model_loader: Union[base.ModelLoader, Callable], return_row=False):

Review Comment:
   If we do this we would really need to document what return_row does.
   
   What I would prefer, is since we can still change our apis we should use one interface or the other.
   
   What do you think of the idea of only using one or the other beam.rows or the PredictionResult dataclass?



##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -17,16 +17,17 @@
 # mypy: ignore-errors
 
 from dataclasses import dataclass
-from typing import Tuple
-from typing import TypeVar
-from typing import Union
+from typing import Callable
 
 import apache_beam as beam
 from apache_beam.ml.inference import base
+from apache_beam.typehints import Tuple

Review Comment:
   I asked @robertwb about this in https://github.com/apache/beam/pull/17762.  He thought that we should prefer native types.  Does that sound right?



##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -52,8 +52,12 @@ class RunInference(beam.PTransform):
 
   TODO(BEAM-14046): Add and link to help documentation
   """
-  def __init__(self, model_loader: base.ModelLoader):
-    self._model_loader = model_loader
+  def __init__(
+      self, model_loader: Union[base.ModelLoader, Callable], return_row=False):
+    self._model_loader = model_loader() if isinstance(

Review Comment:
   I don't get this. Is this for some callable that returns a model loader?  Where would we use this instead of just passing in a model loader?



-- 
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] ihji commented on a diff in pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ihji commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r897329144


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -55,5 +54,9 @@ class RunInference(beam.PTransform):
   def __init__(self, model_loader: base.ModelLoader):
     self._model_loader = model_loader
 
+  @classmethod
+  def create(cls, model_loader_provider, **kwargs):

Review Comment:
   Added a comment.



-- 
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 pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
tvalentyn commented on PR #17773:
URL: https://github.com/apache/beam/pull/17773#issuecomment-1156181271

   fixes: https://github.com/apache/beam/issues/21666


-- 
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] ihji commented on pull request #17773: [BEAM-14524] Add return_row option to RunInference transform

Posted by GitBox <gi...@apache.org>.
ihji commented on PR #17773:
URL: https://github.com/apache/beam/pull/17773#issuecomment-1139148432

   R: @ryanthompson591 


-- 
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] robertwb commented on a diff in pull request #17773: [BEAM-14524] Add return_row option to RunInference transform

Posted by GitBox <gi...@apache.org>.
robertwb commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r891718388


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -52,8 +52,12 @@ class RunInference(beam.PTransform):
 
   TODO(BEAM-14046): Add and link to help documentation
   """
-  def __init__(self, model_loader: base.ModelLoader):
-    self._model_loader = model_loader
+  def __init__(
+      self, model_loader: Union[base.ModelLoader, Callable], return_row=False):

Review Comment:
   As per the discussion, the returned PredictionResult will always be row-compatible rather than requiring an option.



-- 
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] ryanthompson591 commented on a diff in pull request #17773: [BEAM-14524] Returning NamedTuple from RunInference transform

Posted by GitBox <gi...@apache.org>.
ryanthompson591 commented on code in PR #17773:
URL: https://github.com/apache/beam/pull/17773#discussion_r894588323


##########
sdks/python/apache_beam/ml/inference/api.py:
##########
@@ -55,5 +54,9 @@ class RunInference(beam.PTransform):
   def __init__(self, model_loader: base.ModelLoader):
     self._model_loader = model_loader
 
+  @classmethod
+  def create(cls, model_loader_provider, **kwargs):

Review Comment:
   Is this an standard xlang interface that users would expect? I don't really understand it or how it would work.
   
   What do you think about adding a comment to explain why this exists.



-- 
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