-
Notifications
You must be signed in to change notification settings - Fork 692
feat: introduce abstract classes to dynamo services #924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…lwahhab/abstract-depends
|
overall lgtm. need to address few concerns/questions |
…lwahhab/abstract-depends
|
/ok to test bcd47aa |
|
/ok to test bcd47aa |
@mohammedabdulwahhab, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 5d1c926 |
biswapanda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
|
/ok to test ecc6005 |
|
/ok to test 885e6c2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deploy/sdk/src/dynamo/sdk/core/protocol/interface.py (1)
298-339: The decorator reference issue has been resolved.The error message correctly references "@endpoint" which aligns with the actual decorator used in implementations and test expectations.
🧹 Nitpick comments (2)
deploy/sdk/src/dynamo/sdk/core/protocol/interface.py (2)
156-208: Consider refactoring the complex dependency matching logic for better readability.The
linkmethod implementation is functionally correct but has high complexity that could impact maintainability. The nested logic for finding matching dependencies could be extracted into helper methods.Consider refactoring like this:
def link(self, next_service: "ServiceInterface") -> "ServiceInterface": """Link this service to another service, creating a pipeline.""" if not isinstance(next_service, ServiceInterface): raise ValueError(f"link must be passed a Service, got {type(next_service)}") + matching_dep = self._find_matching_dependency(next_service) + matching_dep.on = next_service + LinkedServices.add((self, next_service)) + return next_service +def _find_matching_dependency(self, next_service: "ServiceInterface") -> "DependencyInterface": + """Find the dependency that matches the given service.""" inner_deps = [ (dep.on.inner, dep_key, dep) for dep_key, dep in self.dependencies.items() if dep.on is not None ] curr_inner = next_service.inner matching_deps = [ (dep_inner, dep_key, original_dep) for dep_inner, dep_key, original_dep in inner_deps if issubclass(curr_inner, dep_inner) ] if not matching_deps: raise ValueError( f"{curr_inner.__name__} does not fulfill any dependencies required by {self.name}" ) if len(matching_deps) > 1: dep_names = [dep_key for _, dep_key, _ in matching_deps] raise ValueError( f"{curr_inner.__name__} fulfills multiple dependencies required by {self.name}: {dep_names}" ) + return matching_deps[0][2]
253-275: Update the docstring to match actual implementation.The method comment mentions "@dynamo_endpoint decorators" but the implementation checks for
DynamoEndpointInterfaceinstances. This creates confusion about the validation approach.Apply this diff to clarify the documentation:
def is_servable(self) -> bool: """Check if this service is ready to be served. A service is servable if: 1. It is not a subclass of AbstractService (concrete service) - 2. If it is a subclass of AbstractService, all abstract methods are implemented - with @dynamo_endpoint decorators + 2. If it is a subclass of AbstractService, all abstract endpoints are properly + implemented as DynamoEndpointInterface instances """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deploy/sdk/src/dynamo/sdk/__init__.py(2 hunks)deploy/sdk/src/dynamo/sdk/cli/serving.py(1 hunks)deploy/sdk/src/dynamo/sdk/core/protocol/interface.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/sdk/src/dynamo/sdk/init.py
- deploy/sdk/src/dynamo/sdk/cli/serving.py
🧰 Additional context used
🪛 Pylint (3.3.7)
deploy/sdk/src/dynamo/sdk/core/protocol/interface.py
[refactor] 31-31: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (3)
deploy/sdk/src/dynamo/sdk/core/protocol/interface.py (3)
31-35: LGTM: Well-designed marker interface for abstract services.The
AbstractServiceclass serves as an appropriate marker interface to distinguish abstract service definitions from concrete implementations. The static analysis warning about too few public methods is a false positive for marker interfaces.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 31-31: Too few public methods (0/2)
(R0903)
277-295: LGTM: Clean and focused helper functions.The helper functions
_get_abstract_dynamo_endpointsand_check_dynamo_endpoint_implementedare well-designed with clear responsibilities and appropriate error handling.
372-377: LGTM: Proper abstract setter implementation.The abstract setter for the
onproperty correctly complements the existing getter and enables dependency injection functionality in the concrete implementations.
Overview:
depends(WorkerInterface).link(VllmWorker)Example
See examples/llm_hello_world.py for full reference. Run with
dynamo serve llm_hello_world:Frontend. Condensed version below.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation