fix: remove unused bentoml references #1412
Conversation
biswapanda
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
grahamking,
hhzhang16,
hutm,
ishandhanani,
jthomson04,
julienmancuso,
kkranen,
mohammedabdulwahhab,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25,
tedzhouhk and
tmonty12
as code owners
WalkthroughThis update systematically replaces all references to "bento" and "bentoml" with "dynamo" across the codebase, including schema fields, API routes, environment variables, documentation, comments, and test assertions. The changes ensure consistent terminology and naming conventions, affecting schemas, APIs, CLI interfaces, logging, and deployment logic, without altering core logic or control flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant API
participant Schema
participant Deployment
User->>CLI: Provide dynamo_identifier (was bento_identifier)
CLI->>API: Send request with dynamo fields/routes
API->>Schema: Validate using dynamo_* fields
API->>Deployment: Create/Update deployment with dynamo attribute
Deployment-->>API: Deployment result
API-->>CLI: Response with dynamo-based schema
CLI-->>User: Output result
Poem
🪧 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/serve_dynamo.py (1)
115-115: Fix confusing help text for target option.The help text currently shows "Specify the target: 'dynamo' or 'dynamo'." which presents the same option twice, making it unclear what the valid choices are.
- help="Specify the target: 'dynamo' or 'dynamo'.", + help="Specify the target: 'dynamo'.",Alternatively, if there are multiple valid targets, please specify what the other valid option should be.
deploy/cloud/api-store/ai_dynamo_store/api/utils.py (2)
24-26: Update comments to reflect the new terminology.The comments still reference "BentoML status values" and "BentoML's DeploymentStatus enum" which should be updated to reflect the new "dynamo" terminology for consistency.
- Maps operator status to BentoML status values. - Returns lowercase status values matching BentoML's DeploymentStatus enum. + Maps operator status to Dynamo status values. + Returns lowercase status values matching Dynamo's DeploymentStatus enum.
46-46: Update comment to reflect the new terminology.The comment references "BentoML states" which should be updated for consistency.
- return "deploying" # map pending to deploying to match BentoML states + return "deploying" # map pending to deploying to match Dynamo states🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 43-46: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
deploy/cloud/api-store/ai_dynamo_store/api/components.py(3 hunks)deploy/cloud/api-store/ai_dynamo_store/api/deployments.py(2 hunks)deploy/cloud/api-store/ai_dynamo_store/api/dynamo.py(12 hunks)deploy/cloud/api-store/ai_dynamo_store/api/utils.py(3 hunks)deploy/cloud/api-store/ai_dynamo_store/models/schemas.py(1 hunks)deploy/cloud/api-store/ai_dynamo_store/tests/test_utils.py(2 hunks)deploy/helm/deploy.sh(1 hunks)deploy/sdk/src/dynamo/sdk/__init__.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py(3 hunks)deploy/sdk/src/dynamo/sdk/cli/serving.py(5 hunks)deploy/sdk/src/dynamo/sdk/cli/utils.py(1 hunks)deploy/sdk/src/dynamo/sdk/core/runner/__init__.py(0 hunks)deploy/sdk/src/dynamo/sdk/lib/loader.py(2 hunks)docs/API/sdk.md(1 hunks)lib/bindings/python/src/dynamo/runtime/logging.py(1 hunks)
💤 Files with no reviewable changes (2)
- deploy/sdk/src/dynamo/sdk/init.py
- deploy/sdk/src/dynamo/sdk/core/runner/init.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (28)
deploy/cloud/api-store/ai_dynamo_store/models/schemas.py (1)
59-59:❓ Verification inconclusive
Verify all API consumers are updated for this breaking change.
The field rename from
bentotodynamoinUpdateDeploymentSchemais a breaking change for API clients. Ensure that all consumers of this API have been updated to use the new field name.
🏁 Script executed:
#!/bin/bash # Search for any remaining usage of the old 'bento' field in API requests/responses echo "Searching for 'bento' field usage in API calls..." rg -i '"bento":|\.bento[^a-zA-Z]|bento=' --type py echo "Searching for schema usage..." rg -A 3 -B 3 'UpdateDeploymentSchema' --type pyLength of output: 2414
Verify external API clients and documentation for
dynamofield renameNo internal references to
bentoremain in the codebase (search for"bento"and.bentoreturned zero hits). Before merging, please:
Confirm all external API consumers have been updated to send/parse the new
dynamofield instead ofbento.Update API documentation (OpenAPI schema, README, client SDKs) to reflect the rename.
If you need to support existing clients, consider adding a Pydantic alias:
class UpdateDeploymentSchema(DeploymentConfigSchema):dynamo: Optional[str] = None
dynamo: Optional[str] = Field(None, alias="bento") component: Optional[str] = Nonedeploy/sdk/src/dynamo/sdk/cli/utils.py (1)
53-53:✅ Verification successful
Verify all Protocol implementations are updated for the attribute rename.
The attribute rename from
bentotodynamoinServiceProtocolis a breaking change. All classes implementing this protocol and code accessing this attribute must be updated.
🏁 Script executed:
#!/bin/bash # Search for ServiceProtocol implementations and usage echo "Searching for ServiceProtocol implementations..." rg -A 10 -B 2 'ServiceProtocol' --type py echo "Searching for .bento attribute access..." rg '\.bento[^a-zA-Z]' --type py echo "Searching for dynamo attribute usage to verify consistency..." rg '\.dynamo[^a-zA-Z]' --type pyLength of output: 11999
ServiceProtocol rename from
bentotodynamois complete and safe.No occurrences of the old
.bentoattribute were found, and all usages consistently reference.dynamo. No further updates are required.deploy/helm/deploy.sh (1)
58-58: LGTM! Documentation updated for consistency.The comment update accurately reflects the new terminology and maintains consistency with the broader Dynamo refactoring.
deploy/sdk/src/dynamo/sdk/lib/loader.py (2)
226-226: LGTM! Error message correctly updated to reference "dynamo.yaml".The error message now correctly references the expected configuration file name after the renaming from "bento" to "dynamo".
236-236: LGTM! Comment accurately reflects the directory being processed.The comment now correctly refers to the "dynamo" directory, maintaining consistency with the renaming effort.
docs/API/sdk.md (1)
115-115: LGTM! Documentation generalized appropriately.The removal of the specific "BentoML" reference generalizes the documentation while preserving the technical explanation of the
depends()function's capabilities.deploy/cloud/api-store/ai_dynamo_store/tests/test_utils.py (2)
79-80: LGTM! Test assertions correctly updated for schema changes.The assertions now check for "dynamo" fields instead of "bento" fields, ensuring the tests validate the correct schema structure after the renaming effort.
Also applies to: 90-91, 99-100
96-96: LGTM! Test function name updated consistently.The function name change from
test_build_latest_revision_from_cr_bento_colonlesstotest_build_latest_revision_from_cr_dynamo_colonlessmaintains consistency with the schema field renaming.deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py (2)
97-97: LGTM! Parameter renaming consistently applied.The parameter
bento_identifierhas been correctly renamed todynamo_identifierand its usage is updated accordingly throughout the function.Also applies to: 140-140
101-102: LGTM! Environment variable renaming properly implemented.The environment variable has been correctly renamed from
BENTOML_RUNNER_MAPtoDYNAMO_RUNNER_MAPwith corresponding help text updates.deploy/cloud/api-store/ai_dynamo_store/api/utils.py (1)
75-129: LGTM! Systematic renaming is consistent and complete.The renaming from "bento" to "dynamo" terminology throughout the
build_latest_revision_from_crfunction is well-executed and maintains the original logic while updating all relevant variable names and dictionary keys.deploy/cloud/api-store/ai_dynamo_store/api/deployments.py (5)
88-88: LGTM! Consistent attribute renaming.The change from
deployment.bentotodeployment.dynamocorrectly reflects the updated schema terminology.
94-94: LGTM! Consistent attribute renaming with fallback.The change from
deployment.bento or deployment.componenttodeployment.dynamo or deployment.componentmaintains the fallback logic while using the new attribute name.
326-326: LGTM! Consistent comparison logic.The change from
existing_deployment.bento != deployment.bentotoexisting_deployment.dynamo != deployment.dynamomaintains the validation logic for preventing updates to Dynamo components.
332-332: LGTM! Consistent parameter usage.The change from
deployment.bentotodeployment.dynamoin thesanitize_deployment_namecall is consistent with the schema updates.
336-336: LGTM! Consistent parameter mapping.The change from
dynamo_nim=deployment.bentotodynamo_nim=deployment.dynamocorrectly maps the new attribute to the function parameter.deploy/sdk/src/dynamo/sdk/cli/serving.py (4)
50-55: LGTM! Consistent parameter renaming.The parameter rename from
bento_identifiertodynamo_identifierin_get_dynamo_worker_scriptfunction is consistent and maintains the function's logic.
67-80: LGTM! Consistent parameter renaming across function signature and usage.The parameter rename from
bento_identifiertodynamo_identifierincreate_dynamo_watcherfunction and its usage is consistent with the terminology update.
268-268: LGTM! Comment correction.The comment update from "bento" to "dynamo" improves consistency with the new terminology.
299-299: LGTM! Environment variable renaming is consistent.The change from
"BENTOML_RUNNER_MAP"to"DYNAMO_RUNNER_MAP"aligns with the overall effort to remove BentoML references and adopt Dynamo terminology.deploy/cloud/api-store/ai_dynamo_store/api/components.py (3)
168-168: LGTM! Consistent field renaming in schema.The field rename from
bentoml_versiontodynamo_versioninDynamoComponentVersionManifestSchemais consistent with the terminology update.
182-182: LGTM! Consistent field renaming in schema.The field rename from
bento_repository_uidtodynamo_repository_uidinDynamoComponentVersionSchemamaintains the same data type and validation.
208-210:❓ Verification inconclusive
Verify API compatibility for schema field changes.
The field renames from
latest_bentotolatest_dynamo,latest_bentostolatest_dynamos, andn_bentoston_dynamosare consistent, but these are potentially breaking changes for API consumers.Ensure that all API consumers and client code have been updated to use the new field names, or consider implementing backward compatibility if needed.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old field names in the codebase echo "Searching for potential usage of old field names..." rg -i "latest_bento|latest_bentos|n_bentos|bento_repository_uid|bentoml_version" --type pyLength of output: 205
Verify API compatibility after renaming schema fields
No occurrences of the old field names (
latest_bento,latest_bentos,n_bentos,bento_repository_uid,bentoml_version) were found in the codebase. However, renaming these fields is potentially breaking for external API consumers. Please ensure:
- All client code and downstream services have been updated to use
latest_dynamo,latest_dynamos, andn_dynamos.- If any external integrations still rely on the old names, consider adding backward-compatibility aliases or updating your API documentation.
deploy/cloud/api-store/ai_dynamo_store/api/dynamo.py (5)
125-125: LGTM: API route path renaming is consistent.The renaming of the base API paths from "/bento_repositories" to "/dynamo_repositories" is consistently applied across GET, POST endpoints.
Also applies to: 181-181, 250-250
378-378: LGTM: Nested route segments properly renamed.The renaming of nested route segments from "bentos" to "dynamos" is consistently applied across all CRUD operations (GET, POST, PATCH, PUT) for dynamo component versions.
Also applies to: 418-418, 486-486, 547-547, 591-591, 639-639, 675-675
172-176: LGTM: Schema field names consistently updated.The schema field renaming is semantically correct and consistent:
latest_bento→latest_dynamolatest_bentos→latest_dynamosn_bentos→n_dynamosThese changes maintain the same data structure while aligning with the new terminology.
Also applies to: 243-245, 772-778
835-835: LGTM: Repository reference field properly renamed.The field name change from
bento_repository_uidtodynamo_repository_uidcorrectly reflects the relationship to dynamo components and maintains referential integrity.
125-125:❓ Verification inconclusive
Verify the dual API route structure is intentional.
The codebase maintains both "/dynamo_repositories" and "/dynamo_components" routes that serve identical functionality. This appears to be for backward compatibility or migration purposes.
Also applies to: 133-133, 181-181, 189-189, 250-250, 258-258
🏁 Script executed:
#!/bin/bash # Description: Verify if both route patterns are intentionally maintained across the codebase # Expected: Find documentation or comments explaining the dual route structure echo "Searching for documentation about dual API routes..." rg -A 5 -B 5 "dynamo_repositories.*dynamo_components|backward.compatibility|migration" --type py echo "Checking if both route patterns exist in other API files..." fd -e py -t f . --exec rg -l "dynamo_repositories.*dynamo_components" {} echo "Looking for any configuration or documentation about API versioning..." fd -e md -t f . --exec rg -l "dynamo_repositories|dynamo_components" {}Length of output: 646
🏁 Script executed:
#!/bin/bash # Verify if the “/dynamo_components” route exists anywhere in the codebase echo "Searching for any references to 'dynamo_components' in Python files..." rg -n "dynamo_components" --type pyLength of output: 2788
🏁 Script executed:
#!/bin/bash echo "Searching for 'dynamo_repositories' references in Python files..." rg -n "dynamo_repositories" --type py echo "Searching for 'dynamo_repositories' in markdown documentation..." rg -n "dynamo_repositories" --type mdLength of output: 1544
Confirm intentional dual API routes
I didn’t find any documentation or comments explaining why both
/dynamo_repositoriesand/dynamo_componentsendpoints are maintained—they’re defined side-by-side in deploy/cloud/api-store/ai_dynamo_store/api/dynamo.py (e.g. lines 125 vs. 133, 181 vs. 189, 250 vs. 258, etc.) and consumed by the SDK in deploy/sdk/src/dynamo/sdk/core/deploy/kubernetes.py.
Please verify that this duplication is deliberate (e.g. for backward-compatibility or migration) and either add a note in your API docs/config or consider consolidating the routes if one set is no longer required.
biswapanda
force-pushed
the
bis/refactor_bentoml_refs
branch
from
af5bd54 to
b78c939
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/cloud/api-store/ai_dynamo_store/api/utils.py (2)
46-46: Remove redundantelifafter return
Since the precedingif state == "failed": returnalways exits, you can simplify the next branch to a plainiffor clarity.- if state == "failed": - return "failed" - elif state == "pending": - return "deploying" + if state == "failed": + return "failed" + if state == "pending": + return "deploying"🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 43-46: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
95-101: Consider swappingnamevs.versionfields for clarity
Currently thedynamoobject sets both"name": dynamo_versionand"version": dynamo_version. It may be more intuitive to use"name": dynamo_nameand reserve"version"fordynamo_version.- "name": dynamo_version, + "name": dynamo_name, "resource_type": "dynamo", "labels": [], "description": "", "repository": repository, - "version": dynamo_version, + "version": dynamo_version,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
deploy/cloud/api-store/ai_dynamo_store/api/components.py(3 hunks)deploy/cloud/api-store/ai_dynamo_store/api/deployments.py(2 hunks)deploy/cloud/api-store/ai_dynamo_store/api/dynamo.py(12 hunks)deploy/cloud/api-store/ai_dynamo_store/api/k8s.py(2 hunks)deploy/cloud/api-store/ai_dynamo_store/api/utils.py(5 hunks)deploy/cloud/api-store/ai_dynamo_store/models/schemas.py(1 hunks)deploy/cloud/api-store/ai_dynamo_store/tests/test_utils.py(2 hunks)deploy/helm/deploy.sh(1 hunks)deploy/sdk/src/dynamo/sdk/__init__.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/cli.py(0 hunks)deploy/sdk/src/dynamo/sdk/cli/serve.py(1 hunks)deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py(3 hunks)deploy/sdk/src/dynamo/sdk/cli/serving.py(5 hunks)deploy/sdk/src/dynamo/sdk/cli/utils.py(1 hunks)deploy/sdk/src/dynamo/sdk/core/runner/__init__.py(0 hunks)deploy/sdk/src/dynamo/sdk/lib/loader.py(2 hunks)docs/API/sdk.md(3 hunks)docs/guides/cli_overview.md(2 hunks)examples/llm/configs/disagg.yaml(0 hunks)lib/bindings/python/src/dynamo/runtime/logging.py(1 hunks)
💤 Files with no reviewable changes (4)
- examples/llm/configs/disagg.yaml
- deploy/sdk/src/dynamo/sdk/cli/cli.py
- deploy/sdk/src/dynamo/sdk/init.py
- deploy/sdk/src/dynamo/sdk/core/runner/init.py
✅ Files skipped from review due to trivial changes (1)
- deploy/helm/deploy.sh
🚧 Files skipped from review as they are similar to previous changes (13)
- deploy/cloud/api-store/ai_dynamo_store/models/schemas.py
- lib/bindings/python/src/dynamo/runtime/logging.py
- deploy/sdk/src/dynamo/sdk/lib/loader.py
- deploy/cloud/api-store/ai_dynamo_store/api/k8s.py
- deploy/sdk/src/dynamo/sdk/cli/serve.py
- deploy/sdk/src/dynamo/sdk/cli/utils.py
- docs/guides/cli_overview.md
- deploy/cloud/api-store/ai_dynamo_store/api/deployments.py
- deploy/cloud/api-store/ai_dynamo_store/api/components.py
- deploy/cloud/api-store/ai_dynamo_store/tests/test_utils.py
- deploy/sdk/src/dynamo/sdk/cli/serve_dynamo.py
- deploy/sdk/src/dynamo/sdk/cli/serving.py
- deploy/cloud/api-store/ai_dynamo_store/api/dynamo.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: biswapanda
PR: ai-dynamo/dynamo#1412
File: lib/bindings/python/src/dynamo/runtime/logging.py:100-100
Timestamp: 2025-06-06T21:48:35.177Z
Learning: In the Dynamo codebase, BentoML has been completely removed from all executable code, with only documentation and attribution references remaining. The error_loggers configuration in lib/bindings/python/src/dynamo/runtime/logging.py should not include "bentoml" since those modules no longer exist.
🧬 Code Graph Analysis (1)
deploy/cloud/api-store/ai_dynamo_store/api/utils.py (1)
deploy/sdk/src/dynamo/sdk/cli/deployment.py (1)
get(290-328)
🪛 Pylint (3.3.7)
deploy/cloud/api-store/ai_dynamo_store/api/utils.py
[refactor] 43-46: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
🪛 LanguageTool
docs/API/sdk.md
[uncategorized] ~32-~32: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...'s](https://github.com/bentoml/BentoML) open source deployment patterns. The Dynamo CLI is ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (4)
deploy/cloud/api-store/ai_dynamo_store/api/utils.py (4)
24-25: Docstring aligns with Dynamo terminology
The updated docstring correctly replaces any legacy branding and clearly describes the mapping to Dynamo’sDeploymentStatusvalues.
75-80: Verify CR spec field name and split logic
You’re now readingspec.get("dynamoGraph", ...)and splitting on":". Please confirm that the custom resource indeed uses thedynamoGraphkey (case‐sensitive) and that thename:versionformat is guaranteed.
88-93: Ensurelatest_dynamoinitialization is intentional
The newrepositorydict includes alatest_dynamo: Noneplaceholder. If downstream code relies on this being populated later, confirm that it’s set appropriately, or consider initializing it to the parseddynamo_versioninstead.
128-128: Target object updated to include Dynamo reference
The new"dynamo": dynamoentry on thetargetdict aligns with your renaming effort and ensures the runtime object reflects the correct domain model.
Overview:
Remove unused bento references
Detail in code rabbit summary
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Refactor
Bug Fixes
Documentation