Introduce RemoteModelHandler abstract base class#34379
Introduce RemoteModelHandler abstract base class#34379jrmccluskey merged 10 commits intoapache:masterfrom
Conversation
|
Assigning reviewers. If you would like to opt out of this review, comment R: @liferoad for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Reminder, please take a look at this pr: @liferoad |
jrmccluskey
changed the title
|
I'm happy with where this wound up after design discussion, I think this is ready to review properly. |
| namespace, "cumulativeThrottlingSeconds") | ||
| self.throttler = AdaptiveThrottler( | ||
| window_ms=window_ms, bucket_ms=bucket_ms, overload_ratio=overload_ratio) | ||
| self.logger = logging.getLogger(namespace) |
There was a problem hiding this comment.
does this work if namespace is empty?
There was a problem hiding this comment.
it will work, the logging and the metric will just not be specific to the model handler (for logging that isn't a big deal, but if you hypothetically had multiple distinct remote model handler classes they would share the same cumulativeThrottlingSeconds counter)
There was a problem hiding this comment.
I see. Maybe we can put a docstring about this behavior.
|
|
||
| class RemoteModelHandler(ABC, ModelHandler[ExampleT, PredictionT, ModelT]): | ||
| """Has the ability to call a model at a remote endpoint.""" | ||
| def __init__( |
There was a problem hiding this comment.
just an idea to use config classes since the init arg list could be long (might be not):
from dataclasses import dataclass, field
from typing import Callable
@dataclass
class RemoteModelHandlerConfig:
namespace: str = ''
num_retries: int = 5
throttle_delay_secs: int = 5
retry_filter: Callable[[Exception], bool] = field(default_factory=lambda: lambda x: True)
window_ms: int = 1 * _MILLISECOND_TO_SECOND
bucket_ms: int = 1 * _MILLISECOND_TO_SECOND
overload_ratio: float = 2
```
There was a problem hiding this comment.
I know this is largely coming from a perspective of how to surface this for yaml, but is this configuration consideration actually blocking here? We have to surface all of these parameters at some point
There was a problem hiding this comment.
I do not think it blocks your work. I just want to call out whether we want to do more like https://github.com/apache/beam/blob/master/sdks/python/apache_beam/ml/anomaly/transforms.py#L61. If the arguments keep growing, a config class might be better.
There was a problem hiding this comment.
I mainly ask because you didn't approve the PR
|
LGTM. Thanks. |
* Stash first functioning version * unit tests * Unit tests, documentation * linting and formatting * adjust line-too-long * move external doc to shortlink * create_client + __init_subclass__ * fix exception message to match symbol change * Fix linting * make RemoteModelHandler an explicit abstract base class
2 participants
Implementation of a generic remote model handler abstract base class with small unit tests. The intent is to refactor the existing Vertex AI model handler to use this base class in a follow-up PR, followed by fresh implementations afterwards.
Outlined in a brief design doc - https://docs.google.com/document/d/17A_oHJ7s3ol4TGCUpKeYc6iozkcTwN_e4H_J3kRlgPM/edit?usp=sharing
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.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 or the workflows README to see a list of phrases to trigger workflows.