feat: decouple bento dependency #1266
Conversation
biswapanda
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
grahamking,
hhzhang16,
hutm,
ishandhanani,
julienmancuso,
kkranen,
mohammedabdulwahhab,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
sshchoi,
tanmayv25,
tedzhouhk and
tmonty12
as code owners
WalkthroughThis update removes all BentoML-specific integration and dependency management from the Dynamo SDK. It deletes modules and classes related to BentoML service adaptation, dependency injection, and CLI management of Bento bundles and pipelines. Dockerfiles are simplified by eliminating redundant package management steps. The deployment script and SDK imports are updated to reflect these removals and new interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant DynamoSDK
User->>CLI: Run pipeline/build/serve commands
CLI->>DynamoSDK: Use ServiceInterface and core SDK logic
Note over CLI,DynamoSDK: No BentoML integration or dependency code invoked
DynamoSDK-->>CLI: Returns results (build/serve/deploy)
CLI-->>User: Outputs status/results
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)deploy/sdk/src/dynamo/sdk/tests/test_resources.py (3)
🪛 Pylint (3.3.7)deploy/sdk/src/dynamo/sdk/tests/test_resources.py[warning] 51-51: Redefining name 'setup_and_teardown' from outer scope (line 26) (W0621) [error] 54-54: Unable to import 'dynamo.sdk' (E0401) [convention] 54-54: Import outside toplevel (dynamo.sdk.service) (C0415) [convention] 60-60: Missing class docstring (C0115) [refactor] 60-60: Too few public methods (0/2) (R0903) [error] 65-65: Class 'MockService' has no 'config' member (E1101) [error] 66-66: Class 'MockService' has no 'config' member (E1101) [error] 67-67: Class 'MockService' has no 'config' member (E1101) [error] 68-68: Class 'MockService' has no 'config' member (E1101) [warning] 51-51: Unused argument 'setup_and_teardown' (W0613) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
deploy/sdk/src/dynamo/sdk/cli/cli.py (2)
29-31: Remove commented-out pipeline import and extraneous blank lines
Since thepipeline.pymodule and itsgetcommand have been removed, the commented import (# from dynamo.sdk.cli.pipeline import get) and surrounding blank lines are obsolete. Please delete these lines to clean up dead code.
85-85: Eliminate commented CLI registration ofgetcommand
The registration for thegetcommand (# cli.command()(get)) is commented out. Remove this dead code entry to maintain clarity and ensure the CLI help output accurately reflects available commands.deploy/sdk/src/dynamo/sdk/cli/build.py (1)
132-136: Good fallback mechanism with room for error message improvement.The image fallback logic correctly prioritizes
service.config.imageover theDYNAMO_IMAGEenvironment variable. However, the assertion error message could be more helpful by specifying the exact configuration options.Consider improving the error message:
- assert ( - image is not None - ), "Please set DYNAMO_IMAGE environment variable or image field in service config" + assert ( + image is not None + ), "Image must be specified either via service.config.image or DYNAMO_IMAGE environment variable"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
container/Dockerfile.sglang(0 hunks)container/Dockerfile.tensorrt_llm(0 hunks)container/Dockerfile.vllm(0 hunks)deploy/cloud/operator/internal/controller/dynamocomponentdeployment_controller.go(2 hunks)deploy/helm/deploy.sh(1 hunks)deploy/sdk/src/dynamo/sdk/__init__.py(1 hunks)deploy/sdk/src/dynamo/sdk/cli/allocator.py(2 hunks)deploy/sdk/src/dynamo/sdk/cli/bento_util.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/bentos.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/build.py(2 hunks)deploy/sdk/src/dynamo/sdk/cli/cli.py(2 hunks)deploy/sdk/src/dynamo/sdk/cli/pipeline.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/serving.py(6 hunks)deploy/sdk/src/dynamo/sdk/core/lib.py(0 hunks)deploy/sdk/src/dynamo/sdk/core/protocol/interface.py(2 hunks)deploy/sdk/src/dynamo/sdk/core/runner/bentoml.py(0 hunks)deploy/sdk/src/dynamo/sdk/lib/decorators.py(2 hunks)deploy/sdk/src/dynamo/sdk/lib/dependency.py(0 hunks)deploy/sdk/src/dynamo/sdk/lib/loader.py(6 hunks)deploy/sdk/src/dynamo/sdk/lib/service.py(0 hunks)deploy/sdk/src/dynamo/sdk/tests/test_resources.py(2 hunks)examples/llm/configs/agg_router.yaml(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (11)
- deploy/sdk/src/dynamo/sdk/core/lib.py
- container/Dockerfile.vllm
- container/Dockerfile.sglang
- container/Dockerfile.tensorrt_llm
- deploy/sdk/src/dynamo/sdk/lib/dependency.py
- deploy/sdk/src/dynamo/sdk/cli/bento_util.py
- deploy/sdk/src/dynamo/sdk/lib/service.py
- deploy/sdk/src/dynamo/sdk/cli/bentos.py
- deploy/sdk/src/dynamo/sdk/core/runner/bentoml.py
- deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py
- deploy/sdk/src/dynamo/sdk/cli/pipeline.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
deploy/sdk/src/dynamo/sdk/__init__.py (1)
deploy/sdk/src/dynamo/sdk/lib/decorators.py (2)
async_on_shutdown(102-106)async_on_start(95-99)
deploy/cloud/operator/internal/controller/dynamocomponentdeployment_controller.go (2)
deploy/cloud/operator/internal/consts/consts.go (1)
KubeLabelDynamoComponent(47-47)deploy/cloud/operator/internal/dynamo/graph.go (1)
ComponentTypePlanner(48-48)
deploy/sdk/src/dynamo/sdk/cli/build.py (3)
deploy/sdk/src/dynamo/sdk/core/lib.py (1)
service(53-77)deploy/sdk/src/dynamo/sdk/core/protocol/interface.py (4)
config(116-118)ServiceConfig(73-81)name(89-91)name(110-112)deploy/sdk/src/dynamo/sdk/core/runner/dynamo.py (3)
config(111-112)name(61-62)name(107-108)
deploy/sdk/src/dynamo/sdk/cli/serving.py (1)
deploy/sdk/src/dynamo/sdk/lib/loader.py (1)
find_and_load_service(35-84)
deploy/sdk/src/dynamo/sdk/core/protocol/interface.py (2)
deploy/sdk/src/dynamo/sdk/cli/build.py (1)
ServiceConfig(97-109)deploy/cloud/operator/internal/dynamo/graph.go (2)
ServiceConfig(87-91)DynamoConfig(53-58)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (18)
examples/llm/configs/agg_router.yaml (1)
43-44: LGTM! Resource specifications properly added.The explicit CPU and memory specifications are correctly formatted for Kubernetes resource management and align well with the existing GPU allocation. This standardization supports the broader resource configuration improvements in the SDK.
pyproject.toml (1)
197-197: LGTM! Mypy configuration correctly updated for BentoML removal.The mypy module skip configuration has been appropriately narrowed to only skip
vllm.*modules, removing the previously skipped BentoML-related modules (bentoml.*,fs.*,_bentoml_sdk.*). This aligns perfectly with the BentoML dependency removal and ensures proper type checking for the remaining codebase.deploy/sdk/src/dynamo/sdk/__init__.py (1)
26-26: LGTM! Import successfully migrated from BentoML to Dynamo SDK.The import of
async_on_shutdownhas been correctly moved from BentoML todynamo.sdk.lib.decorators, maintaining consistency withasync_on_startbeing imported from the same module. The implementation indecorators.pyproperly sets the__dynamo_shutdown_hook__attribute, ensuring the decorator functionality is preserved while removing the BentoML dependency.deploy/helm/deploy.sh (1)
90-90: LGTM! Deployment path correctly updated for Dynamo package structure.The Helm values file path has been properly updated from the BentoML-specific structure (
~/bentoml/bentos/...) to the new Dynamo package structure (~/.dynamo/packages/...). The file name change frombento.yamltodynamo.yamlis consistent with the BentoML decoupling objective and aligns with the new package management approach.deploy/sdk/src/dynamo/sdk/tests/test_resources.py (2)
19-19: LGTM! Import successfully migrated from BentoML to Dynamo's service interface.This change aligns with the PR objective to decouple from BentoML dependencies by replacing the import with Dynamo's own
ServiceInterface.
44-48: Enhanced test assertions provide better validation.The test has been improved by:
- Adding proper type annotation for
dyn_svc- Expanding from a single assertion to multiple granular checks
- Validating specific resource configuration fields (
cpu,gpu,memory)This provides more comprehensive testing of the updated resource configuration model.
deploy/sdk/src/dynamo/sdk/cli/allocator.py (2)
26-26: LGTM! Successfully replaced BentoML service import.The import has been correctly updated from
_bentoml_sdkto use Dynamo's ownServiceInterfacefrom the protocol interface module, consistent with the PR's decoupling objective.
156-156: Type annotation properly updated to match the new service interface.The parameter type has been correctly changed from
Service[Any]toServiceInterface[Any], maintaining type safety while using the new Dynamo service interface.deploy/cloud/operator/internal/controller/dynamocomponentdeployment_controller.go (1)
1458-1460: Conditional logic correctly restricts environment argument to Planner components.The conditional check properly uses the component label to determine when to append the environment argument, restricting it to Planner component deployments as intended. The logic flow is clear and follows the expected pattern.
deploy/sdk/src/dynamo/sdk/lib/decorators.py (3)
26-26: Good addition of type variable for improved typing precision.The
Ftype variable with proper callable bounds enhances type safety for decorator functions, providing better type inference and IDE support.
95-100: Enhanced type annotation improves decorator typing.The
async_on_startdecorator now uses the bounded type variableFinstead of the generict.Callable, providing more precise type information while maintaining the same functionality.
102-106: Newasync_on_shutdowndecorator follows consistent pattern.The implementation mirrors the
async_on_startdecorator structure:
- Uses the same type variable for proper typing
- Sets the appropriate attribute marker (
__dynamo_shutdown_hook__)- Returns the original function unchanged
This addition supports the BentoML decoupling effort by providing shutdown lifecycle hooks within Dynamo's own decorator library.
deploy/sdk/src/dynamo/sdk/cli/build.py (2)
141-141: Field name corrected to match interface updates.The change from
resourcetoresourcescorrectly aligns with theServiceConfig.resourcesfield definition in the interface module.
595-595: Explicit string conversion ensures subprocess compatibility.Converting
output_pathto string prevents potential issues with subprocess.run when dealing with Path objects.deploy/sdk/src/dynamo/sdk/lib/loader.py (1)
29-29: Consistent migration to ServiceInterface.All service type references have been consistently updated to use
ServiceInterfacefrom the protocol interface module. The changes maintain the same logic while aligning with the new service abstraction architecture.Also applies to: 38-38, 87-87, 142-142, 181-181, 212-212
deploy/sdk/src/dynamo/sdk/core/protocol/interface.py (2)
62-70: Improved resource configuration with type standardization and validation.The changes standardize all resource fields (
cpu,memory,gpu) to string types, which provides consistency for Kubernetes-style resource specifications. The field validator forgpuensures backward compatibility by converting integer inputs to strings.The memory default increase from "100Mi" to "500Mi" provides a more reasonable default allocation.
77-77:✅ Verification successful
Field renamed for consistency across the codebase.
The rename from
resourcetoresourcesaligns with the usage in other modules and follows common naming conventions for collections.Verify that all references to the old
resourcefield have been updated throughout the codebase:
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining references to the old 'resource' field # that should be updated to 'resources' # Search for service.config.resource patterns rg -A 2 -B 2 "\.config\.resource[^s]" --type py # Search for resource field access in ServiceConfig contexts rg -A 2 -B 2 "ServiceConfig.*resource[^s]" --type pyLength of output: 104
🏁 Script executed:
#!/bin/bash # Re-verify removal of all singular 'resource' field references # 1. Match .config.resource at end-of-word or end-of-line rg -E "\.config\.resource(\b|$)" --type py # 2. Match ServiceConfig.resource at end-of-word or end-of-line rg -E "ServiceConfig\.resource(\b|$)" --type py # 3. Catch any .resource usages excluding .resources rg -E "\.resource\b" --type py | grep -v "\.resources"Length of output: 1342
🏁 Script executed:
#!/bin/bash # Corrected searches for any remaining references to the singular 'resource' field # 1. Search for ".config.resource" at word boundary or end-of-line rg '\.config\.resource(\b|$)' -t py # 2. Search for "ServiceConfig.resource" at word boundary or end-of-line rg 'ServiceConfig\.resource(\b|$)' -t py # 3. Catch any ".resource" usages excluding ".resources" rg '\.resource\b' -t py | grep -v '\.resources'Length of output: 211
✅ All singular
.resourceusages have been removedNo lingering references to the old
resourcefield were found in attribute contexts. The only.resourcematch is in an import statement indeploy/sdk/src/dynamo/sdk/cli/allocator.py, which refers to the module name and doesn’t impact the renamed field. Rename is verified and safe to approve.deploy/sdk/src/dynamo/sdk/cli/serving.py (1)
155-155: Interface simplified by removing polymorphic input support.The function now exclusively accepts string pipeline identifiers instead of supporting both strings and service instances. This simplification removes BentoML-specific abstractions and makes the interface more predictable.
The parameter renaming from
bento_identifiertodynamo_pipelinebetter reflects the current architecture and is consistently applied throughout the function.Also applies to: 176-176, 237-237, 255-255, 411-411
deploy/cloud/operator/internal/controller/dynamocomponentdeployment_controller.go
Show resolved
Hide resolved
biswapanda
force-pushed
the
bis/decouple_bento_deps
branch
from
2a6ab5f to
8de9fd0
Compare
|
/ok to test 8de9fd0 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
deploy/sdk/src/dynamo/sdk/__init__.py (1)
19-21: Remove outdated BentoML warning filter and TODO comment.Since BentoML dependency has been decoupled as part of this PR, the warning filter for BentoML setuptools warnings and its associated TODO comment are no longer needed.
Apply this diff to remove the outdated code:
-# Suppress warning from setuptools caused by bentoml -# TODO: Remove this line after the bentoml import is removed from this file -warnings.filterwarnings("ignore", category=UserWarning, message=".*pkg_resources.*")🧰 Tools
🪛 Pylint (3.3.7)
[warning] 20-20: TODO: Remove this line after the bentoml import is removed from this file
(W0511)
deploy/sdk/src/dynamo/sdk/cli/allocator.py (1)
24-24: Import resolution issue persists (duplicate)This import error was already identified and detailed guidance provided in a previous review. Please follow the recommendations to fix the module resolution by ensuring proper
__init__.pyfiles or configuring the package structure correctly.🧰 Tools
🪛 Pylint (3.3.7)
[error] 24-24: Unable to import 'dynamo.sdk.core.protocol.interface'
(E0401)
deploy/sdk/src/dynamo/sdk/lib/loader.py (1)
29-29: Same import resolution issue as allocator.pyThe
dynamo.sdk.core.protocol.interfacemodule cannot be resolved. This is the same issue flagged inallocator.pyand needs to be addressed across both files consistently.Verify that the interface module structure is properly configured:
#!/bin/bash # Check if interface.py exists and verify package structure fd -t f "interface.py" deploy/sdk/src/dynamo/sdk/core/protocol/ # Check for __init__.py files in the path find deploy/sdk/src/dynamo -name "__init__.py" | sort # Try importing the module in Python cd deploy/sdk/src && python -c "from dynamo.sdk.core.protocol.interface import ServiceInterface; print('Import successful')" 2>&1 || echo "Import failed"🧰 Tools
🪛 Pylint (3.3.7)
[error] 29-29: Unable to import 'dynamo.sdk.core.protocol.interface'
(E0401)
🧹 Nitpick comments (3)
deploy/sdk/src/dynamo/sdk/__init__.py (2)
16-16: Consider removing unused warnings import.If the BentoML warning filter is removed as suggested above, the
warningsimport on line 16 will become unused and should also be removed.-import warnings
26-26: Import order and path verification.The import of decorators from
dynamo.sdk.lib.decoratorsis functionally correct (as verified in previous reviews), but there are two style considerations:
- Pylint suggests placing this import at the top of the module for consistency
- The static analysis import error appears to be a false positive since the module was previously verified to exist
Consider moving this import closer to the top after the typing import to address the import order convention:
import warnings from typing import Any +from dynamo.sdk.lib.decorators import async_on_shutdown, async_on_start # Suppress warning from setuptools caused by bentoml # TODO: Remove this line after the bentoml import is removed from this file warnings.filterwarnings("ignore", category=UserWarning, message=".*pkg_resources.*") # flake8: noqa: E402 from dynamo.sdk.core.decorators.endpoint import api, endpoint from dynamo.sdk.core.lib import DYNAMO_IMAGE, depends, liveness, readiness, service -from dynamo.sdk.lib.decorators import async_on_shutdown, async_on_start🧰 Tools
🪛 Pylint (3.3.7)
[error] 26-26: Unable to import 'dynamo.sdk.lib.decorators'
(E0401)
[convention] 26-26: Import "from dynamo.sdk.lib.decorators import async_on_shutdown, async_on_start" should be placed at the top of the module
(C0413)
deploy/sdk/src/dynamo/sdk/lib/loader.py (1)
87-196: Consider refactoring for reduced complexity (optional)The
_do_importfunction has high complexity (21 local variables, 21 branches, 63 statements). While this isn't introduced by the current changes, consider breaking this function into smaller, focused helper functions:
_handle_file_path_import()for file-based imports_find_root_service()for service discovery logic_navigate_service_attributes()for attribute traversalThis would improve maintainability and testability.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 87-87: Too many local variables (21/15)
(R0914)
[warning] 90-90: Use lazy % formatting in logging functions
(W1203)
[warning] 100-100: Use lazy % formatting in logging functions
(W1203)
[warning] 125-125: Use lazy % formatting in logging functions
(W1203)
[warning] 127-127: Use lazy % formatting in logging functions
(W1203)
[warning] 131-131: Use lazy % formatting in logging functions
(W1203)
[warning] 134-134: Consider explicitly re-raising using 'raise ImportError(f'Failed to import module "{module_name}": {e}') from e'
(W0707)
[warning] 144-144: Use lazy % formatting in logging functions
(W1203)
[warning] 159-159: Use lazy % formatting in logging functions
(W1203)
[warning] 174-174: Use lazy % formatting in logging functions
(W1203)
[warning] 177-177: Use lazy % formatting in logging functions
(W1203)
[warning] 182-182: Use lazy % formatting in logging functions
(W1203)
[warning] 185-185: Use lazy % formatting in logging functions
(W1203)
[warning] 188-188: Consider explicitly re-raising using 'except (AttributeError, KeyError) as exc' and 'raise ValueError(f'Attribute "{attr}" not found in "{module_name}"') from exc'
(W0707)
[warning] 193-193: Use lazy % formatting in logging functions
(W1203)
[refactor] 87-87: Too many branches (21/12)
(R0912)
[refactor] 87-87: Too many statements (63/50)
(R0915)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
container/Dockerfile.sglang(0 hunks)container/Dockerfile.tensorrt_llm(0 hunks)container/Dockerfile.vllm(0 hunks)deploy/helm/deploy.sh(1 hunks)deploy/sdk/src/dynamo/sdk/__init__.py(1 hunks)deploy/sdk/src/dynamo/sdk/cli/allocator.py(2 hunks)deploy/sdk/src/dynamo/sdk/cli/bento_util.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/bentos.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/build.py(1 hunks)deploy/sdk/src/dynamo/sdk/cli/cli.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/pipeline.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/serving.py(7 hunks)deploy/sdk/src/dynamo/sdk/core/runner/bentoml.py(0 hunks)deploy/sdk/src/dynamo/sdk/lib/decorators.py(2 hunks)deploy/sdk/src/dynamo/sdk/lib/dependency.py(0 hunks)deploy/sdk/src/dynamo/sdk/lib/loader.py(6 hunks)deploy/sdk/src/dynamo/sdk/lib/service.py(0 hunks)examples/tensorrt_llm/common/utils.py(3 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (11)
- deploy/sdk/src/dynamo/sdk/cli/cli.py
- container/Dockerfile.sglang
- container/Dockerfile.tensorrt_llm
- deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py
- container/Dockerfile.vllm
- deploy/sdk/src/dynamo/sdk/lib/dependency.py
- deploy/sdk/src/dynamo/sdk/lib/service.py
- deploy/sdk/src/dynamo/sdk/cli/bento_util.py
- deploy/sdk/src/dynamo/sdk/core/runner/bentoml.py
- deploy/sdk/src/dynamo/sdk/cli/pipeline.py
- deploy/sdk/src/dynamo/sdk/cli/bentos.py
✅ Files skipped from review due to trivial changes (2)
- deploy/helm/deploy.sh
- examples/tensorrt_llm/common/utils.py
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/sdk/src/dynamo/sdk/cli/build.py
- pyproject.toml
- deploy/sdk/src/dynamo/sdk/lib/decorators.py
- deploy/sdk/src/dynamo/sdk/cli/serving.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
deploy/sdk/src/dynamo/sdk/__init__.py (1)
deploy/sdk/src/dynamo/sdk/lib/decorators.py (2)
async_on_shutdown(102-106)async_on_start(95-99)
🪛 Pylint (3.3.7)
deploy/sdk/src/dynamo/sdk/__init__.py
[error] 26-26: Unable to import 'dynamo.sdk.lib.decorators'
(E0401)
[convention] 26-26: Import "from dynamo.sdk.lib.decorators import async_on_shutdown, async_on_start" should be placed at the top of the module
(C0413)
deploy/sdk/src/dynamo/sdk/cli/allocator.py
[error] 24-24: Unable to import 'dynamo.sdk.core.protocol.interface'
(E0401)
deploy/sdk/src/dynamo/sdk/lib/loader.py
[error] 29-29: Unable to import 'dynamo.sdk.core.protocol.interface'
(E0401)
[refactor] 87-87: Too many local variables (21/15)
(R0914)
[refactor] 87-87: Too many branches (21/12)
(R0912)
[refactor] 87-87: Too many statements (63/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (2)
deploy/sdk/src/dynamo/sdk/cli/allocator.py (1)
153-153: LGTM: Type annotation correctly updatedThe type annotation change from
Service[Any]toServiceInterface[Any]is consistent with the import change and maintains type safety while supporting the BentoML decoupling objective.deploy/sdk/src/dynamo/sdk/lib/loader.py (1)
38-38: LGTM: Consistent interface adoptionAll type annotations and
isinstancechecks have been correctly updated to useServiceInterfaceinstead of the previousDynamoService. The changes maintain the same logic flow while supporting the BentoML decoupling objective through proper abstraction.Also applies to: 87-87, 142-142, 181-181, 212-212
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deploy/sdk/src/dynamo/sdk/__init__.py (1)
19-21: Remove outdated BentoML warning suppressionSince BentoML imports have been removed, this warning filter is no longer needed and should be cleaned up.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 20-20: TODO: Remove this line after the bentoml import is removed from this file
(W0511)
🧹 Nitpick comments (4)
examples/hello_world/hello_world.py (1)
91-93: LGTM: Consistent shutdown hook implementation across servicesThe
@on_shutdowndecorators and shutdown methods are correctly implemented across all three services, providing proper lifecycle management.Consider adding docstrings to the shutdown methods for consistency:
@on_shutdown def shutdown(self): + """Clean up backend resources on shutdown.""" logger.info("Shutting down backend")Also applies to: 120-122, 165-167
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 92-92: Missing function or method docstring
(C0116)
docs/API/sdk.md (1)
174-174: Minor grammar improvement suggestionConsider adding a comma for better readability.
-The `@on_shutdown` hook is called when the service is shutdown handles cleanup. +The `@on_shutdown` hook is called when the service is shutdown, and handles cleanup.🧰 Tools
🪛 LanguageTool
[uncategorized] ~174-~174: Possible missing comma found.
Context: ...own` hook is called when the service is shutdown handles cleanup. ```python @on_shutdow...(AI_HYDRA_LEO_MISSING_COMMA)
deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py (2)
314-325: Improve logging formatting and exception specificity.The exit handler implementation is solid for graceful shutdown, but consider addressing these minor improvements:
Apply this diff to address static analysis warnings:
- logger.error(f"Error running shutdown hooks: {e}") + logger.error("Error running shutdown hooks: %s", e)The broad exception catching is appropriate here since this is a shutdown scenario where we want to ensure the process can exit even if shutdown hooks fail.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 321-321: Catching too general exception Exception
(W0718)
[warning] 322-322: Use lazy % formatting in logging functions
(W1203)
330-337: Improve signal handler implementation.The signal handling logic is correct, but consider these improvements:
Apply this diff to address static analysis warnings:
- def signal_handler(signum, frame): - logger.info(f"Received signal {signum}, initiating graceful shutdown") + def signal_handler(signum, _frame): + logger.info("Received signal %s, initiating graceful shutdown", signum)Using
_frameindicates the parameter is intentionally unused, and lazy logging formatting is more efficient.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 331-331: Use lazy % formatting in logging functions
(W1203)
[warning] 330-330: Unused argument 'frame'
(W0613)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
deploy/sdk/src/dynamo/sdk/__init__.py(1 hunks)deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py(3 hunks)deploy/sdk/src/dynamo/sdk/core/protocol/interface.py(1 hunks)deploy/sdk/src/dynamo/sdk/lib/decorators.py(2 hunks)docs/API/sdk.md(4 hunks)examples/hello_world/hello_world.py(4 hunks)examples/llm/components/frontend.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/sdk/src/dynamo/sdk/core/protocol/interface.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/sdk/src/dynamo/sdk/lib/decorators.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
deploy/sdk/src/dynamo/sdk/__init__.py (1)
deploy/sdk/src/dynamo/sdk/lib/decorators.py (2)
async_on_start(95-99)on_shutdown(102-106)
🪛 Pylint (3.3.7)
examples/llm/components/frontend.py
[error] 27-27: Unable to import 'dynamo.sdk'
(E0401)
[convention] 27-27: third party import "dynamo.sdk.api" should be placed before first party imports "components.planner_service.Planner", "components.processor.Processor", "components.worker.VllmWorker"
(C0411)
deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py
[warning] 321-321: Catching too general exception Exception
(W0718)
[warning] 322-322: Use lazy % formatting in logging functions
(W1203)
[warning] 331-331: Use lazy % formatting in logging functions
(W1203)
[warning] 330-330: Unused argument 'frame'
(W0613)
deploy/sdk/src/dynamo/sdk/__init__.py
[error] 26-26: Unable to import 'dynamo.sdk.lib.decorators'
(E0401)
[convention] 26-26: Import "from dynamo.sdk.lib.decorators import async_on_start, on_shutdown" should be placed at the top of the module
(C0413)
examples/hello_world/hello_world.py
[convention] 92-92: Missing function or method docstring
(C0116)
[convention] 121-121: Missing function or method docstring
(C0116)
[convention] 166-166: Missing function or method docstring
(C0116)
🪛 LanguageTool
docs/API/sdk.md
[uncategorized] ~174-~174: Possible missing comma found.
Context: ...own` hook is called when the service is shutdown handles cleanup. ```python @on_shutdow...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~473-~473: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... dependency from the Processor and only spin up the Router.
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (9)
examples/llm/components/frontend.py (2)
27-27: LGTM: Import transition from BentoML to native decoratorThe import change from
async_on_shutdowntoon_shutdowncorrectly implements the decoupling from BentoML dependency as specified in the PR objectives.🧰 Tools
🪛 Pylint (3.3.7)
[error] 27-27: Unable to import 'dynamo.sdk'
(E0401)
[convention] 27-27: third party import "dynamo.sdk.api" should be placed before first party imports "components.planner_service.Planner", "components.processor.Processor", "components.worker.VllmWorker"
(C0411)
123-137: LGTM: Decorator change aligns with synchronous cleanup logicThe transition from
@async_on_shutdownto@on_shutdownis appropriate since the cleanup method contains only synchronous operations (subprocess.run). This change successfully removes the BentoML dependency while maintaining the same functionality.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 127-127: Line too long (118/100)
(C0301)
deploy/sdk/src/dynamo/sdk/__init__.py (2)
26-26: LGTM: Import transition successfully implementedThe import change to
dynamo.sdk.lib.decoratorscorrectly implements the native decorator functionality, removing the BentoML dependency as intended.🧰 Tools
🪛 Pylint (3.3.7)
[error] 26-26: Unable to import 'dynamo.sdk.lib.decorators'
(E0401)
[convention] 26-26: Import "from dynamo.sdk.lib.decorators import async_on_start, on_shutdown" should be placed at the top of the module
(C0413)
32-32: LGTM: Export list correctly updatedThe export of
on_shutdowninstead ofasync_on_shutdownis consistent with the decorator transition.examples/hello_world/hello_world.py (1)
28-28: LGTM: Import added for shutdown lifecycle hookThe import of
on_shutdownenables the lifecycle hook functionality for graceful service shutdown.docs/API/sdk.md (1)
86-91: LGTM: Documentation correctly updated for decorator transitionThe documentation accurately reflects the change from
@async_on_shutdownto@on_shutdownand shows the synchronous shutdown method signature, which aligns with the implementation changes.Also applies to: 173-181
deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py (3)
21-21: LGTM: Standard library imports for shutdown handling.The imports for
atexitandsignalare appropriate for implementing the new graceful shutdown mechanism.Also applies to: 26-26
327-327: LGTM: Proper exit handler registration.The
atexit.registercall correctly ensures shutdown hooks run on normal process exit.
360-366: LGTM: Well-implemented synchronous shutdown hooks mechanism.The
run_shutdown_hooksfunction correctly iterates through class methods and executes those marked with the__dynamo_shutdown_hook__attribute. This synchronous approach aligns with the broader effort to decouple from BentoML's async shutdown mechanisms.
ajcasagrande
left a comment
ajcasagrande
left a comment
There was a problem hiding this comment.
code looks good to me, have not tried to run it.
nv-anants
left a comment
nv-anants
left a comment
There was a problem hiding this comment.
dockerfile and toml changes lgtm
Overview:
Decouple dynamo from bentoml as a hard dependency
Where should the reviewer start?
More details in
#1266 (comment)
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Chores
New Features