[Serve] Reimplement BackendConfig as pydantic model#10389
[Serve] Reimplement BackendConfig as pydantic model#10389edoakes merged 43 commits intoray-project:masterfrom
Conversation
edoakes
left a comment
There was a problem hiding this comment.
Looks really nice, I'm excited! A few smallish things to clean up.
python/ray/serve/config.py
Outdated
| max_batch_size: Optional[PositiveInt] = None | ||
| batch_wait_timeout: float = 0 | ||
| max_concurrent_queries: Optional[int] = None | ||
| autoscaling_config: Optional[Dict[str, Any]] = None |
There was a problem hiding this comment.
Can we avoid exposing this to users for now somehow? Else we should mark it as experimental somehow.
Also, is it possible to add a description for each of these fields that will be auto-documented?
There was a problem hiding this comment.
You're just talking about the autoscaling_config? Would putting an underscore before the name be enough?
For the auto-documentation, yes, I'll add it. And the autodoc will also hide variables whose name starts with an underscore.
There was a problem hiding this comment.
It turns out that with our current version of Sphinx (1.8.5), there's no way to autodocument type hints, nor is there a way to autodocument default values for instance attributes. So I'll just use a docstring for the class BackendConfig.
|
@simon-mo All |
Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>
python/ray/serve/config.py
Outdated
| max_batch_size: Optional[PositiveInt] = None | ||
| batch_wait_timeout: float = 0 | ||
| max_concurrent_queries: Optional[int] = None | ||
| autoscaling_config: Optional[Dict[str, Any]] = None |
python/ray/serve/config.py
Outdated
| # This is not a pydantic validator, so that we may skip this method when | ||
| # creating partially filled BackendConfig objects to pass as updates--for | ||
| # example, BackendConfig(max_batch_size=5). | ||
| def _validate_complete(self): |
There was a problem hiding this comment.
- let's call it _validate_max_batch_size or something similar?
There was a problem hiding this comment.
It's true that currently max_batch_size is the only thing being validated here, but potentially we could put any kind of validation here that we only want to run on completely filled BackendConfigs. If we call it _validate_max_batch_size(), I worry it might look kind of arbitrary when we call it elsewhere in serve--like, why are they only validating this specific parameter?
Would putting this logic in _validate_max_batch_size() and then making _validate_complete() call _validate_max_batch_size() be reasonable?
|
I think one alternative is to use string literal as types. Maybe we can try that: |
|
Lint is failing, can you add pydantic to requirements-doc? |
Why are these changes needed?
Previously the user of the API would pass in an untyped dict to configure the backend. This PR exposes a typed
BackendConfigclass to the user. We use pydantic for type hints and validation.This is a work in progress. What remains to be done is to fully update the Serve documentation for this change.
Related issue number
Checks
scripts/format.shto lint the changes in this PR.