Refactor API code to base.py in RunInference #21801
Conversation
|
Can one of the admins verify this patch? |
4 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Looks good.
@ihji this will probably change what you do with your PR.
| api.PredictionResult(numpy.array([1, 2, 3]), 6), | ||
| api.PredictionResult(numpy.array([4, 5, 6]), 15), | ||
| api.PredictionResult(numpy.array([7, 8, 9]), 24) | ||
| base.PredictionResult(numpy.array([1, 2, 3]), 6), |
There was a problem hiding this comment.
WDYT of just importing PredictionResult directly like we do in the pytorch test.
There was a problem hiding this comment.
yep. we can do that.
| @@ -14,6 +14,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| # | |||
| # mypy: ignore-errors | |||
There was a problem hiding this comment.
What was the error here? was it something like Type variable is unbound?
There was a problem hiding this comment.
Do we understand why we hit that error?
There was a problem hiding this comment.
I think #21441 describes a different problem: supporting type inference for RunInference API users. I believe some of the approaches, that it mentions, have been implemented (adding generics), and we have some type inference capabilities (that you plan to test in another test). I'd ask, what is the remaining work there?
The problem of having to disable mypy in this file seems to do with developer tooling. Do we know what causes mypy to complain? Does this problem need a separate issue?
There was a problem hiding this comment.
These are the errors mypy complains about.
apache_beam/testing/load_tests/load_test_metrics_utils.py:51: error: unused 'type: ignore' comment
apache_beam/ml/inference/base.py:68: error: Type variable "apache_beam.ml.inference.base._INPUT_TYPE" is unbound [valid-type]
apache_beam/ml/inference/base.py:68: note: (Hint: Use "Generic[_INPUT_TYPE]" or "Protocol[_INPUT_TYPE]" base class to bind "_INPUT_TYPE" inside a class)
apache_beam/ml/inference/base.py:68: note: (Hint: Use "_INPUT_TYPE" in function signature to bind "_INPUT_TYPE" inside a function)
apache_beam/ml/inference/base.py:69: error: Type variable "apache_beam.ml.inference.base._OUTPUT_TYPE" is unbound [valid-type]
apache_beam/ml/inference/base.py:69: note: (Hint: Use "Generic[_OUTPUT_TYPE]" or "Protocol[_OUTPUT_TYPE]" base class to bind "_OUTPUT_TYPE" inside a class)
apache_beam/ml/inference/base.py:69: note: (Hint: Use "_OUTPUT_TYPE" in function signature to bind "_OUTPUT_TYPE" inside a function
There was a problem hiding this comment.
Find an issue related to this. python/mypy#7520.
There was a problem hiding this comment.
Looking into these errors, this is more related to mypy and Dataclass[1]. Since we are moving to use NamedTuple instead of Dataclass, can we move forward with this?
[1] python/mypy#7520. To solve the current error of mypy with dataclass, PredictionResult should be modified as below
@dataclass
class PredictionResult(Generic[_INPUT_TYPE, _OUTPUT_TYPE]):
...
Got it. Will update my PR accordingly after this PR gets merged. |
|
PTAL @tvalentyn |
| If examples are paired with keys, it will output a tuple | ||
| (key, PredictionResult) for each (key, example) input. | ||
|
|
||
| Models for supported frameworks can be loaded via a URI. Supported services |
There was a problem hiding this comment.
TODO in a separate PR:
- proof-read these API bits by TW.
- Supported services can also be used - what is this about?
- TODO(BEAM-14046): Add and link to help documentation : Let's point to the examples directory.
|
merging based on results from previous commit run. |
* refactor code from api to base * delete api.py * modify imports * Add todo to mypy github issue * Refactor code to reflect changes of apache#21777 * Refactor example with KeyedModelHandler * remove explicit type hints from RunInference class * Fixup : Lint * remove TODO to github issue for mypy error * Add mypy github issue as TODO
Refactor code from api to base.py. Delete api.py
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).CHANGES.mdwith noteworthy changes.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.