Skip to content

Conversation

@mohammedabdulwahhab
Copy link
Contributor

@mohammedabdulwahhab mohammedabdulwahhab commented May 1, 2025

Overview:

  • Declare dependencies on interfaces of a component rather than the real thing: depends(WorkerInterface)
  • Use link to inject concrete implementation of the dependency when composing your pipeline. .link(VllmWorker)

Example

See examples/llm_hello_world.py for full reference. Run with dynamo serve llm_hello_world:Frontend. Condensed version below.

class WorkerInterface(AbstractService):
    """Interface for LLM workers."""
    
    @abstract_endpoint
    async def generate(self, request: ChatRequest):
        pass

class RouterInterface(AbstractService):
    """Interface for request routers."""
    
    @abstract_endpoint
    async def generate(self, request: ChatRequest):
        pass

# 2. Implement concrete workers
@service(dynamo={"namespace": "llm-hello-world"})
class VllmWorker(WorkerInterface):
    @endpoint()
    async def generate(self, request: ChatRequest):
        # Convert to Spongebob case
        for token in request.text.split():
            yield token.upper() if random() < 0.5 else token.lower()

@service(dynamo={"namespace": "llm-hello-world"})
class TRTLLMWorker(WorkerInterface):
    @endpoint()
    async def generate(self, request: ChatRequest):
        # Convert to SHOUTING case
        for token in request.text.split():
            yield token.upper()

# 3. Implement concrete routers that depend on workers
@service(dynamo={"namespace": "llm-hello-world"})
class FastRouter(RouterInterface):
    worker = depends(WorkerInterface)  # Can use any WorkerInterface implementation

    @endpoint()
    async def generate(self, request: ChatRequest):
        async for response in self.worker.generate(request):
            await asyncio.sleep(0.1)  # Fast routing
            yield response

@service(dynamo={"namespace": "llm-hello-world"})
class SlowRouter(RouterInterface):
    worker = depends(WorkerInterface)  # Can use any WorkerInterface implementation

    @endpoint()
    async def generate(self, request: ChatRequest):
        async for response in self.worker.generate(request):
            await asyncio.sleep(1.0)  # Slow routing
            yield response

# 4. Frontend service that depends on routers
@service(dynamo={"namespace": "llm-hello-world"})
class Frontend:
    router = depends(RouterInterface)  # Can use any RouterInterface implementation

    @api()
    async def generate(self, request: ChatRequest):
        async for response in self.router.generate(request):
            yield response

# 5. Compose different pipelines through linking
fast_vllm_pipeline = Frontend.link(FastRouter).link(VllmWorker)
fast_trt_pipeline = Frontend.link(FastRouter).link(TRTLLMWorker)
slow_vllm_pipeline = Frontend.link(SlowRouter).link(VllmWorker)

Summary by CodeRabbit

  • New Features

    • Introduced support for abstract service interfaces and abstract endpoints in the Dynamo SDK, enabling more flexible service composition.
    • Added new example: "LLM Hello World" pipeline demonstrating abstract interfaces, multiple worker and router implementations, and FastAPI integration with streaming responses.
    • Added a new data model for chat requests in the examples.
  • Bug Fixes

    • Improved dependency handling and validation for abstract services to ensure correct service linking and readiness checks.
  • Refactor

    • Updated service linking and dependency assignment mechanisms for greater flexibility and correctness.
    • Streamlined configuration handling in the Planner example service.
  • Documentation

    • Added detailed comments and usage examples in the new LLM Hello World pipeline.

@copy-pr-bot
Copy link

copy-pr-bot bot commented May 1, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mohammedabdulwahhab mohammedabdulwahhab marked this pull request as ready for review May 5, 2025 17:41
@biswapanda
Copy link
Contributor

overall lgtm. need to address few concerns/questions
#924 (comment)

@mohammedabdulwahhab
Copy link
Contributor Author

/ok to test bcd47aa

@mohammedabdulwahhab
Copy link
Contributor Author

/ok to test bcd47aa

@copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 4, 2025

/ok to test bcd47aa

@mohammedabdulwahhab, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@mohammedabdulwahhab
Copy link
Contributor Author

/ok to test 5d1c926

Copy link
Contributor

@biswapanda biswapanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@mohammedabdulwahhab
Copy link
Contributor Author

/ok to test ecc6005

@mohammedabdulwahhab
Copy link
Contributor Author

/ok to test 885e6c2

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 link method 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 DynamoEndpointInterface instances. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5d1c926 and 885e6c2.

📒 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 AbstractService class 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_endpoints and _check_dynamo_endpoint_implemented are well-designed with clear responsibilities and appropriate error handling.


372-377: LGTM: Proper abstract setter implementation.

The abstract setter for the on property correctly complements the existing getter and enables dependency injection functionality in the concrete implementations.

@mohammedabdulwahhab mohammedabdulwahhab merged commit b293b45 into main Jun 5, 2025
7 checks passed
@mohammedabdulwahhab mohammedabdulwahhab deleted the mabdulwahhab/abstract-depends branch June 5, 2025 05:40
This was referenced Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants