Update bentoml integration to 1.3.5 and add containerization #3045
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve significant updates to the deployment of models using BentoML within the ZenML framework. Enhancements include improved documentation, updated method signatures, and the introduction of new classes and configuration options for local and containerized deployments. The overall structure of the deployment services has been refined to support various service types, enhancing flexibility and usability. Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
|
Classification template updates in |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
NLP template updates in |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (28)
examples/mlops_starter/requirements.txt (1)
Line range hint
1-6: Consider adding version constraints for all dependencies.While zenml[server] has a version constraint, other dependencies including the newly added pandas don't have any. To ensure reproducibility and avoid potential compatibility issues, it's recommended to specify version constraints for all dependencies.
Here's a suggested update to the requirements.txt file:
zenml[server]>=0.50.0 -notebook -scikit-learn -pyarrow -pandas +notebook==6.5.4 +scikit-learn==1.2.2 +pyarrow==12.0.0 +pandas==2.0.2Note: The versions suggested are the latest stable versions as of the current date. Please adjust these according to your specific needs and compatibility requirements.
examples/e2e_nlp/requirements.txt (2)
4-4: Approve addition of 'datasets' package and suggest documentationThe addition of the 'datasets' package (version >=2.12.0,<3.0.0) is appropriate and aligns with the PR objectives. This package will likely enhance dataset handling capabilities in the project.
Consider adding a comment in the requirements.txt file or updating the project documentation to explain the purpose and usage of the 'datasets' package in the context of this project. This will help other developers understand why this dependency was added.
'accelerate' package still in use after removal from requirements
The
acceleratepackage has been removed fromexamples/e2e_nlp/requirements.txt, but it is still being imported in the following files:
tests/integration/integrations/huggingface/steps/helpers.pytests/integration/integrations/huggingface/steps/test_accelerate_runner.pysrc/zenml/utils/function_utils.pyexamples/llm_finetuning/steps/finetune.pyexamples/llm_finetuning/utils/loaders.pyexamples/llm_finetuning/utils/callbacks.pyPlease ensure that the removal of
acceleratewas intentional. If the package is still required, consider adding it back torequirements.txt. Otherwise, refactor the code to eliminate its usage.🔗 Analysis chain
Line range hint
1-5: Verify the intentional removal of 'accelerate' packageThe 'accelerate' package has been removed from the requirements. This package is often used to improve the performance of deep learning models, especially on GPU.
Please confirm if this removal was intentional and if there are any plans to replace its functionality. Consider running the following script to check for any remaining usage of 'accelerate' in the codebase:
If there are no matches, it confirms that 'accelerate' is no longer used in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of 'accelerate' in Python files rg --type python 'import\s+accelerate|from\s+accelerate\s+import'Length of output: 98
Script:
#!/bin/bash # Search for any remaining usage of 'accelerate' in Python files rg --type py 'import\s+accelerate|from\s+accelerate\s+import'Length of output: 584
examples/mlops_starter/configs/feature_engineering.yaml (1)
6-6: Approved: Addition of pandas to required integrationsThe addition of 'pandas' to the required integrations is appropriate for feature engineering tasks. Pandas is a powerful library for data manipulation and analysis, which aligns well with the purpose of this configuration file.
Consider grouping related libraries together. You might want to move 'pyarrow' from
requirementstorequired_integrationssince it's often used alongside pandas for efficient data handling. This would make the configuration more consistent:required_integrations: - sklearn - pandas - pyarrowThis change would centralize all the main data processing libraries under
required_integrations.examples/mlops_starter/configs/inference.yaml (1)
6-6: LGTM! Consider adding a comment for clarity.The addition of 'pandas' to the required integrations is appropriate and aligns with the PR objectives of updating the BentoML integration and adding containerization support. This change suggests an enhancement in data manipulation capabilities for the inference process.
Consider adding a brief comment explaining why pandas is now required. This would improve maintainability and help future developers understand the purpose of this addition.
settings: docker: required_integrations: - sklearn + # Required for advanced data manipulation in the inference process - pandas requirements: - pyarrowexamples/mlops_starter/configs/training_rf.yaml (1)
6-6: LGTM! Consider updating documentation if necessary.The addition of 'pandas' to the required integrations is appropriate and consistent with common data science workflows. This change suggests that the pipeline now utilizes pandas functionality alongside sklearn.
Consider updating any related documentation or README files to reflect this new dependency, ensuring that users are aware of the pandas requirement when setting up the environment.
tests/integration/examples/bentoml/steps/deployer.py (2)
22-22: LGTM. Consider enhancing the comment.The port change from 3001 to 3000 looks good. To improve clarity, consider updating the comment to include the reason for this specific port choice, if there is one.
- port=3000, # Port to be used by the http server + port=3000, # Port to be used by the http server (default for many web services)
25-25: LGTM. Consider enhancing the comment for clarity.The addition of the
deployment_typeparameter is a good improvement, allowing for flexible testing of different deployment scenarios. This aligns well with the PR's objective of introducing containerization.To improve clarity, consider expanding the comment to explain the default behavior and the implications of each option:
- deployment_type="container", # change to "local" to run locally + deployment_type="container", # Options: "container" (default) for containerized deployment, "local" for local executiontests/integration/examples/bentoml/steps/bento_builder.py (1)
Line range hint
1-34: Consider enhancing test structure and documentationWhile this file sets up a
bento_builderstep with comprehensive parameters, it lacks explicit test functions or assertions. To improve adherence to PyTest best practices and enhance clarity:
- Consider adding explicit test function(s) using the
def test_*():format.- Include assertions to verify the expected behavior of the
bento_builderstep.- Add docstrings or comments to explain the purpose and expected outcomes of the test(s).
These additions would make the test more robust and self-documenting, aligning better with PyTest best practices.
tests/integration/examples/bentoml/service.py (3)
21-28: Approve class structure and suggest adding docstrings.The class-based approach improves code organization, and the constructor correctly initializes the model. However, docstrings are missing for both the class and the constructor.
Consider adding docstrings to improve code documentation:
@bentoml.service( name=SERVICE_NAME, ) class MNISTService: """BentoML service for MNIST model predictions.""" def __init__(self): """Initialize the MNIST service by loading the model.""" # load model self.model = bentoml.pytorch.load_model(MODEL_NAME) self.model.eval()🧰 Tools
🪛 Ruff
24-24: Missing docstring in public class
(D101)
25-25: Missing docstring in
__init__(D107)
30-41: Approvepredict_ndarraymethod and suggest adding a docstring.The
predict_ndarraymethod is well-implemented with proper type annotations and input processing. However, a docstring is missing.Consider adding a docstring to improve method documentation:
@bentoml.api() async def predict_ndarray( self, inp: Annotated[np.ndarray, DType("float32"), Shape((28, 28))] ) -> np.ndarray: """ Predict the digit from a numpy array representation of an MNIST image. Args: inp (np.ndarray): A 28x28 float32 numpy array representing the image. Returns: np.ndarray: The model's prediction output. """ # Method implementation...🧰 Tools
🪛 Ruff
31-31: Missing docstring in public method
(D102)
43-54: Approvepredict_imagemethod and suggest adding a docstring.The
predict_imagemethod is well-implemented with proper input processing and type annotations. However, a docstring is missing.Consider adding a docstring to improve method documentation:
@bentoml.api() async def predict_image(self, f: PILImage) -> np.ndarray: """ Predict the digit from a PIL Image representation of an MNIST image. Args: f (PILImage): A 28x28 PIL Image representing the MNIST digit. Returns: np.ndarray: The model's prediction output. """ # Method implementation...🧰 Tools
🪛 Ruff
44-44: Missing docstring in public method
(D102)
tests/integration/examples/bentoml/steps/predictor.py (3)
27-27: LGTM: Function signature update improves flexibility.The change to use
Union[BentoMLLocalDeploymentService, BentoMLContainerDeploymentService]for theserviceparameter type is a good improvement. It allows the function to work with both local and container deployment services, enhancing its flexibility.Consider updating the function's docstring to include a description for the
inference_dataparameter:def predictor( inference_data: Dict[str, List], service: Union[BentoMLLocalDeploymentService, BentoMLContainerDeploymentService], ) -> None: """Run an inference request against the BentoML prediction service. Args: inference_data: A dictionary containing inference data. service: The BentoML service (local or container). """
35-36: LGTM: Improved service start logic.The addition of the
if not service.is_runningcheck is a good improvement. It prevents unnecessary service starts, which can improve efficiency. The comment# should be a NOP if already startedprovides useful context.Consider making the timeout value configurable, either as a function parameter or a class attribute. This would provide more flexibility for different deployment scenarios:
def predictor( inference_data: Dict[str, List], service: Union[BentoMLLocalDeploymentService, BentoMLContainerDeploymentService], service_start_timeout: int = 10, ) -> None: # ... if not service.is_running: service.start(timeout=service_start_timeout) # should be a NOP if already started # ...
Misplaced implementation code in tests directory.
The file
tests/integration/examples/bentoml/steps/predictor.pycontains implementation code rather than test code. Additionally, no corresponding test files (e.g.,test_predictor.py) were found to validate its functionality.Consider the following actions:
- If intended as implementation code: Move
predictor.pyto an appropriatesrcorexamplesdirectory.- If intended as a test file: Rename it following PyTest conventions (e.g.,
test_predictor.py) and add appropriate test functions with PyTest decorators.🔗 Analysis chain
Line range hint
1-67: Clarify the purpose and location of this file.This file (
tests/integration/examples/bentoml/steps/predictor.py) appears to contain implementation code rather than test code, despite being located in atestsdirectory. There are no explicit test functions or PyTest decorators visible.To ensure this file is correctly placed and named, please run the following command:
If this file is indeed meant to be an implementation file, consider moving it to an appropriate
srcorexamplesdirectory. If it's intended to be a test file, rename it to follow PyTest conventions (e.g.,test_predictor.py) and add appropriate test functions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test files related to predictor.py # Expected result: Find test files that actually contain PyTest functions testing the predictor functionality # Search for test files related to predictor.py echo "Searching for test files related to predictor.py:" fd -t f "test_.*predictor.*\.py" tests # If no files found, search for any test files in the bentoml integration if [ $? -ne 0 ]; then echo "No specific test files found for predictor.py. Searching for any test files in the bentoml integration:" fd -t f "test_.*\.py" tests/integration/examples/bentoml fi # Display content of found test files echo "Content of found test files:" fd -t f "test_.*\.py" tests/integration/examples/bentoml --exec cat {}Length of output: 309
🧰 Tools
🪛 Ruff
25-25: Missing argument description in the docstring for
predictor:inference_data(D417)
examples/mlops_starter/README.md (2)
54-54: LGTM: Integration installation command updated.The integration installation command has been expanded to include 'pandas', which is likely needed for the MLOps starter example. This ensures users have all necessary dependencies installed.
Consider adding a brief comment explaining why 'pandas' is now required, to provide context for the change. For example:
- zenml integration install sklearn -y + # Install required integrations (sklearn for machine learning, pandas for data manipulation) + zenml integration install sklearn pandas -y
51-51: Consider removing trailing punctuation from heading.Markdownlint has flagged a trailing punctuation issue in a heading on this line. While this isn't part of the changes in this PR, it's a good practice to address such issues for overall document quality.
Consider removing the trailing colon from the heading. For example:
- #### Option 1 - Interactively explore the quickstart using Jupyter Notebook: + #### Option 1 - Interactively explore the quickstart using Jupyter Notebook🧰 Tools
🪛 Markdownlint
51-51: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
src/zenml/cli/served_model.py (1)
218-223: LGTM! Consider a minor improvement for consistency.The addition of
prediction_apis_urlsenhances the output by providing valuable information about prediction API URLs. The implementation follows the existing pattern and includes a proper fallback message.For consistency with the existing code style, consider using a ternary operator for the fallback message:
-prediction_apis_urls = ( - model_deployer.get_model_server_info(served_models[0]).get( - "PREDICTION_APIS_URLS" - ) - or "No prediction APIs URLs specified for this service" -) +prediction_apis_urls = model_deployer.get_model_server_info(served_models[0]).get( + "PREDICTION_APIS_URLS" +) or "No prediction APIs URLs specified for this service"This change would make the code more concise and align with the style used for
prediction_hostname.Also applies to: 228-229
docs/book/component-guide/model-deployers/bentoml.md (6)
9-12: Approved with a minor suggestion.The updates provide clearer information about BentoML's capabilities and integration with ZenML. The added warning about deployment paths and
bentoctldeprecation is valuable for users.Consider adding a comma after "Provided with the BentoML integration" in line 9 for improved readability:
-The BentoML Model Deployer is one of the available flavors of the [Model Deployer](./model-deployers.md) stack component. Provided with the BentoML integration it can be used to deploy and [manage BentoML models](https://docs.bentoml.org/en/latest/guides/model-store.html#manage-models) or [Bento](https://docs.bentoml.org/en/latest/reference/stores.html#manage-bentos) on a local running HTTP server. +The BentoML Model Deployer is one of the available flavors of the [Model Deployer](./model-deployers.md) stack component. Provided with the BentoML integration, it can be used to deploy and [manage BentoML models](https://docs.bentoml.org/en/latest/guides/model-store.html#manage-models) or [Bento](https://docs.bentoml.org/en/latest/reference/stores.html#manage-bentos) on a local running HTTP server.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Possible missing comma found.
Context: ...ck component. Provided with the BentoML integration it can be used to deploy and [manage Be...(AI_HYDRA_LEO_MISSING_COMMA)
52-88: Approved with a minor suggestion for consistency.The updated BentoML Service creation example is more comprehensive and demonstrates modern BentoML features. The use of type annotations and async methods is a good practice, and covering both numpy array and image inputs is useful for users.
For consistency, consider adding a return type annotation to the
predict_imagemethod:- async def predict_image(self, f: PILImage) -> np.ndarray: + async def predict_image(self, f: PILImage) -> np.ndarray:This matches the style used in the
predict_ndarraymethod and improves code consistency.
Line range hint
94-120: Approved with a minor grammatical suggestion.The updated Bento Builder step section provides clear instructions on how to use the
bento_builder_stepwithin a ZenML pipeline. The example is comprehensive and includes important parameters. The note about flexibility is valuable for users understanding the deployment options.There's a minor grammatical issue in line 94. Consider revising it as follows:
-Once you have your bento service defined, we can use the built-in bento builder step to build the bento bundle that will be used to serve the model. The following example shows how can call the built-in bento builder step within a ZenML pipeline. +Once you have your bento service defined, we can use the built-in bento builder step to build the bento bundle that will be used to serve the model. The following example shows how you can call the built-in bento builder step within a ZenML pipeline.This change improves the sentence structure and clarity.
Line range hint
124-191: Excellent addition with a suggestion for clarity.The new BentoML Deployer step section is a valuable addition to the documentation. It clearly explains both local and containerized deployment options, providing examples for each. The information about Docker image creation and usage is particularly helpful for users interested in containerized deployment.
To improve clarity, consider adding a brief explanation of when users might choose local vs. containerized deployment. This could be added after line 125, for example:
The choice between local and containerized deployment depends on your use case: - Local deployment is simpler and suitable for development and testing. - Containerized deployment is more portable and better suited for production environments or when you need to ensure consistency across different systems.This addition would help users make an informed decision about which deployment method to use.
🧰 Tools
🪛 LanguageTool
[typographical] ~152-~152: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ht registry name as prefix to the image name, otherwise the image push will fail. ```python fr...(THUS_SENTENCE)
🪛 Markdownlint
173-173: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
174-174: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
148-148: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
197-220: Approved with a suggestion for consistency.The updated ZenML BentoML Pipeline example provides a comprehensive view of a typical ML pipeline using BentoML and ZenML. The inclusion of the
deployment_typeparameter effectively demonstrates the new containerized deployment option. The example is clear and well-structured.For consistency with the rest of the document, consider adding a brief comment explaining the
deployment_type="container"parameter in thedeployerstep. This could be added as a comment in the code example:bento = bento_builder(model=model) - deployer(deploy_decision=decision, bento=bento, deployment_type="container") + deployer( + deploy_decision=decision, + bento=bento, + deployment_type="container" # Use containerized deployment + )This addition would help reinforce the concept of containerized deployment introduced earlier in the document.
🧰 Tools
🪛 LanguageTool
[typographical] ~152-~152: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ht registry name as prefix to the image name, otherwise the image push will fail. ```python fr...(THUS_SENTENCE)
🪛 Markdownlint
173-173: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
174-174: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
148-148: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
1-290: Overall excellent improvements with a suggestion for final polish.The document has been significantly enhanced with new information, examples, and clearer explanations. The additions regarding containerized deployment and the updated pipeline examples are particularly valuable. The structure flows logically, guiding the reader from basic concepts to more advanced deployment scenarios.
To further improve the document, consider:
- Conducting a final proofreading pass to address minor grammatical and stylistic issues throughout the document.
- Ensuring consistent formatting of code blocks, especially indentation in Python examples.
- Reviewing the use of emphasis (bold and italic) to ensure it's applied consistently and effectively.
- Double-checking all links to ensure they are up-to-date and functional.
These final touches will elevate the overall quality and professionalism of the documentation.
🧰 Tools
🪛 LanguageTool
[typographical] ~152-~152: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ht registry name as prefix to the image name, otherwise the image push will fail. ```python fr...(THUS_SENTENCE)
🪛 Markdownlint
173-173: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
174-174: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
148-148: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
tests/integration/examples/bentoml/steps/prediction_service_loader.py (1)
59-62: Avoid hardcoding service type names; use constants or enums instead.Comparing the service type using a hardcoded string (
"bentoml-container-deployment") may be error-prone. Consider defining service type names as constants or using an enumeration to enhance code robustness and maintainability.src/zenml/integrations/bentoml/services/bentoml_local_deployment.py (1)
207-207: Replace single quotes with double quotes in string literalsAccording to the project's coding style guidelines, double quotes are preferred over single quotes for string literals.
Apply this diff to adhere to the style guide:
- svc = load(bento_identifier=self.config.bento_tag, working_dir=self.config.working_dir or '.') + svc = load(bento_identifier=self.config.bento_tag, working_dir=self.config.working_dir or ".") ... - working_dir=self.config.working_dir or '.', + working_dir=self.config.working_dir or ".",Also applies to: 239-239
🧰 Tools
🪛 Ruff
207-207: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
src/zenml/integrations/bentoml/model_deployers/bentoml_model_deployer.py (1)
Line range hint
105-115: Update docstring to reflect parameter type change.The
get_model_server_infomethod now acceptsservice_instanceasBaseService, but the docstring still refers to it as a "BentoML deployment service object". To maintain clarity and accuracy, please update the docstring to reflect the new parameter type and expected instances.Consider modifying the docstring as follows:
Args: - service_instance: BentoML deployment service object + service_instance: A BentoML service instance (`BentoMLLocalDeploymentService` or `BentoMLContainerDeploymentService`).src/zenml/integrations/bentoml/services/bentoml_container_deployment.py (1)
1-1: Add a module-level docstring to describe the module's purposeIt's recommended to include a docstring at the top of the module to provide a summary of its purpose and contents.
Consider adding a module docstring like:
+"""Module for deploying BentoML services as Docker containers within the ZenML framework."""🧰 Tools
🪛 Ruff
1-1: Missing docstring in public module
(D100)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (25)
- docs/book/component-guide/model-deployers/bentoml.md (7 hunks)
- docs/mocked_libs.json (1 hunks)
- examples/e2e_nlp/.copier-answers.yml (1 hunks)
- examples/e2e_nlp/config.yaml (0 hunks)
- examples/e2e_nlp/requirements.txt (1 hunks)
- examples/mlops_starter/.copier-answers.yml (1 hunks)
- examples/mlops_starter/README.md (3 hunks)
- examples/mlops_starter/configs/feature_engineering.yaml (1 hunks)
- examples/mlops_starter/configs/inference.yaml (1 hunks)
- examples/mlops_starter/configs/training_rf.yaml (1 hunks)
- examples/mlops_starter/configs/training_sgd.yaml (1 hunks)
- examples/mlops_starter/quickstart.ipynb (1 hunks)
- examples/mlops_starter/requirements.txt (1 hunks)
- src/zenml/cli/served_model.py (1 hunks)
- src/zenml/integrations/bentoml/model_deployers/bentoml_model_deployer.py (8 hunks)
- src/zenml/integrations/bentoml/services/init.py (1 hunks)
- src/zenml/integrations/bentoml/services/bentoml_container_deployment.py (1 hunks)
- src/zenml/integrations/bentoml/services/bentoml_local_deployment.py (8 hunks)
- src/zenml/integrations/bentoml/steps/bento_builder.py (1 hunks)
- src/zenml/integrations/bentoml/steps/bentoml_deployer.py (7 hunks)
- tests/integration/examples/bentoml/service.py (1 hunks)
- tests/integration/examples/bentoml/steps/bento_builder.py (1 hunks)
- tests/integration/examples/bentoml/steps/deployer.py (1 hunks)
- tests/integration/examples/bentoml/steps/prediction_service_loader.py (2 hunks)
- tests/integration/examples/bentoml/steps/predictor.py (1 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- examples/e2e_nlp/config.yaml
✅ Files skipped from review due to trivial changes (3)
- examples/e2e_nlp/.copier-answers.yml
- examples/mlops_starter/.copier-answers.yml
- examples/mlops_starter/quickstart.ipynb
🧰 Additional context used
📓 Path-based instructions (13)
docs/book/component-guide/model-deployers/bentoml.md (1)
Pattern
docs/**/*.md: Review the documentation for readability and clarity.src/zenml/cli/served_model.py (1)
Pattern
src/zenml/**/*.py: Review the Python code for conformity with Python best practices.src/zenml/integrations/bentoml/model_deployers/bentoml_model_deployer.py (1)
Pattern
src/zenml/**/*.py: Review the Python code for conformity with Python best practices.src/zenml/integrations/bentoml/services/__init__.py (1)
Pattern
src/zenml/**/*.py: Review the Python code for conformity with Python best practices.src/zenml/integrations/bentoml/services/bentoml_container_deployment.py (1)
Pattern
src/zenml/**/*.py: Review the Python code for conformity with Python best practices.src/zenml/integrations/bentoml/services/bentoml_local_deployment.py (1)
Pattern
src/zenml/**/*.py: Review the Python code for conformity with Python best practices.src/zenml/integrations/bentoml/steps/bento_builder.py (1)
Pattern
src/zenml/**/*.py: Review the Python code for conformity with Python best practices.src/zenml/integrations/bentoml/steps/bentoml_deployer.py (1)
Pattern
src/zenml/**/*.py: Review the Python code for conformity with Python best practices.tests/integration/examples/bentoml/service.py (1)
Pattern
tests/**/*.py: "Assess the unit test code employing the PyTest testing framework. Confirm that:
- The tests adhere to PyTest's established best practices.
- Test descriptions are sufficiently detailed to clarify the purpose of each test."
tests/integration/examples/bentoml/steps/bento_builder.py (1)
Pattern
tests/**/*.py: "Assess the unit test code employing the PyTest testing framework. Confirm that:
- The tests adhere to PyTest's established best practices.
- Test descriptions are sufficiently detailed to clarify the purpose of each test."
tests/integration/examples/bentoml/steps/deployer.py (1)
Pattern
tests/**/*.py: "Assess the unit test code employing the PyTest testing framework. Confirm that:
- The tests adhere to PyTest's established best practices.
- Test descriptions are sufficiently detailed to clarify the purpose of each test."
tests/integration/examples/bentoml/steps/prediction_service_loader.py (1)
Pattern
tests/**/*.py: "Assess the unit test code employing the PyTest testing framework. Confirm that:
- The tests adhere to PyTest's established best practices.
- Test descriptions are sufficiently detailed to clarify the purpose of each test."
tests/integration/examples/bentoml/steps/predictor.py (1)
Pattern
tests/**/*.py: "Assess the unit test code employing the PyTest testing framework. Confirm that:
- The tests adhere to PyTest's established best practices.
- Test descriptions are sufficiently detailed to clarify the purpose of each test."
🪛 LanguageTool
docs/book/component-guide/model-deployers/bentoml.md
[uncategorized] ~9-~9: Possible missing comma found.
Context: ...ck component. Provided with the BentoML integration it can be used to deploy and [manage Be...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~94-~94: It appears that a pronoun is missing.
Context: ... the model. The following example shows how can call the built-in bento builder ste...(WHERE_MD_VB)
[uncategorized] ~94-~94: Possible missing comma found.
Context: ...you have the bento service file in your repository and then use the correct class name in ...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~120-~120: Consider replacing ‘gives’ with a different word to let your writing stand out.
Context: ...ing using thebentoctlor Yatai. This gives you the flexibility to package your model i...(GIVE_TIME_STYLE)
[typographical] ~152-~152: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ht registry name as prefix to the image name, otherwise the image push will fail. ```python fr...(THUS_SENTENCE)
🪛 Markdownlint
docs/book/component-guide/model-deployers/bentoml.md
130-130: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
173-173: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
174-174: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
148-148: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
examples/mlops_starter/README.md
51-51: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
🪛 Ruff
src/zenml/integrations/bentoml/model_deployers/bentoml_model_deployer.py
138-138: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
src/zenml/integrations/bentoml/services/bentoml_container_deployment.py
1-1: Missing docstring in public module
(D100)
279-279: Local variable
svcis assigned to but never usedRemove assignment to unused variable
svc(F841)
src/zenml/integrations/bentoml/services/bentoml_local_deployment.py
207-207: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
239-239: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
tests/integration/examples/bentoml/service.py
17-17: Missing docstring in public function
(D103)
24-24: Missing docstring in public class
(D101)
25-25: Missing docstring in
__init__(D107)
31-31: Missing docstring in public method
(D102)
44-44: Missing docstring in public method
(D102)
tests/integration/examples/bentoml/steps/predictor.py
25-25: Missing argument description in the docstring for
predictor:inference_data(D417)
🔇 Additional comments (30)
examples/mlops_starter/requirements.txt (1)
5-5: LGTM: Addition of pandas dependency.The inclusion of pandas is a good addition to the project. It's a powerful library for data manipulation and analysis, which aligns well with the existing dependencies like scikit-learn and pyarrow.
examples/mlops_starter/configs/training_sgd.yaml (1)
6-6: Addition of pandas to required integrations is appropriate.The inclusion of pandas in the list of required integrations for the Docker environment is a good addition. Pandas is a powerful data manipulation library commonly used in data science and machine learning workflows.
This change suggests that the pipeline now utilizes pandas functionality, which can enhance data preprocessing, analysis, or feature engineering steps. It's consistent with the existing sklearn integration and aligns well with the machine learning context of this configuration.
However, to ensure this change is fully integrated:
- Verify that pandas is actually used in the associated Python scripts.
- Check if any documentation needs updating to reflect this new dependency.
- Ensure that the Docker image build process accommodates this new requirement.
To confirm the usage of pandas in the project, run the following script:
This will help verify that pandas is indeed used in the project and that this addition to the required integrations is necessary.
✅ Verification successful
Addition of pandas to required integrations is validated.
The verification confirms that
pandasis extensively used throughout the codebase, justifying its inclusion in the required integrations. This ensures that all necessary dependencies are appropriately listed for the project's functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for pandas usage in Python files # Search for pandas import statements echo "Checking for pandas imports:" rg --type python 'import\s+pandas|from\s+pandas\s+import' # Search for common pandas operations (df, read_csv, etc.) echo -e "\nChecking for common pandas operations:" rg --type python '\b(df|read_csv|DataFrame|Series)\b'Length of output: 337
Script:
#!/bin/bash # Description: Check for pandas usage in Python files # Search for pandas import statements echo "Checking for pandas imports:" rg --type py 'import\s+pandas|from\s+pandas\s+import' # Search for common pandas operations (df, read_csv, etc.) echo -e "\nChecking for common pandas operations:" rg --type py '\b(df|read_csv|DataFrame|Series)\b'Length of output: 34310
tests/integration/examples/bentoml/steps/deployer.py (1)
22-25: Overall, the changes look good and align with the PR objectives.The updates to the
bentoml_model_deployer_stepconfiguration enhance the flexibility of the deployment process in the test environment. The changes allow for testing both containerized and local deployments, which is valuable for comprehensive testing.While this file doesn't contain actual test code, it sets up the configuration for tests, which is crucial for effective testing. To further improve the test suite:
- Ensure that there are corresponding test cases that utilize both the "container" and "local" deployment types.
- Consider adding a test case that verifies the correct application of these configuration parameters.
These additions would strengthen the test coverage and align even more closely with PyTest best practices.
src/zenml/integrations/bentoml/services/__init__.py (2)
16-22: LGTM: Import changes reflect new deployment structureThe import changes appropriately reflect the shift from general deployment to specific local and container deployments. The use of absolute imports and
# noqacomments for potentially unused imports in__init__.pyis consistent with Python best practices.
25-30: LGTM:__all__list correctly updatedThe
__all__list has been properly updated to reflect the new local and container deployment classes, removing the old general deployment classes. This change ensures that users of this module will have access to the correct classes, maintaining consistency with the import changes.tests/integration/examples/bentoml/steps/bento_builder.py (1)
23-23: Service parameter updated to use MNISTServiceThe change from
"service.py:svc"to"service.py:MNISTService"aligns with the PR objectives of updating the BentoML integration. This modification appears to reference a more specific service class (MNISTService) instead of a generic service.To ensure this change is consistent across the codebase, please run the following script:
tests/integration/examples/bentoml/service.py (2)
1-20: LGTM: Imports and helper function are appropriate.The new imports, including
Annotated,DType, andShape, are correctly added to support the type annotations in the refactored code. Theto_numpyhelper function remains unchanged and is still relevant for the service implementation.🧰 Tools
🪛 Ruff
1-1: Missing docstring in public module
(D100)
17-17: Missing docstring in public function
(D103)
1-54: Overall approval: Excellent refactoring with minor documentation improvements needed.The refactoring of this file to use a class-based approach for the MNIST service is a significant improvement. It enhances code organization, readability, and maintainability. The addition of type annotations, especially for input validation, is a great step towards improving type safety.
Key improvements:
- Class-based structure for better encapsulation
- Use of
Annotatedfor input validation inpredict_ndarray- Consistent async methods for predictions
To further enhance the code:
- Add docstrings to the class and methods as suggested in previous comments
- Consider adding error handling for potential exceptions during model prediction
Great job on the refactoring! These changes will make the code more robust and easier to maintain.
🧰 Tools
🪛 Ruff
1-1: Missing docstring in public module
(D100)
17-17: Missing docstring in public function
(D103)
24-24: Missing docstring in public class
(D101)
25-25: Missing docstring in
__init__(D107)
31-31: Missing docstring in public method
(D102)
44-44: Missing docstring in public method
(D102)
tests/integration/examples/bentoml/steps/predictor.py (1)
14-21: LGTM: Import changes are appropriate and well-organized.The new imports for
Union,BentoMLLocalDeploymentService, andBentoMLContainerDeploymentServiceare correctly added and necessary for the updated function signature and type hints. The organization of imports follows Python conventions.src/zenml/integrations/bentoml/steps/bento_builder.py (1)
96-96: Approved: Explicit model specification enhances clarity and controlThe addition of
models=[model_name]to thebentos.buildcall is a positive change. It explicitly specifies which model should be included in the BentoML bundle, enhancing clarity and providing better control over the packaging process. This modification aligns well with the PR objective of updating the BentoML integration and improves the overall functionality of thebento_builder_step.docs/mocked_libs.json (1)
47-47: LGTM: New module added for mockingThe addition of "deepchecks.tabular.checks.model_evaluation" to the list of mocked libraries is appropriate and correctly placed. This change likely supports documentation generation for new features or tests involving model evaluation checks from the deepchecks library.
examples/mlops_starter/README.md (3)
27-27: LGTM: Colab link updated correctly.The Colab link has been updated to point to the correct location of the MLOps starter example notebook. This change ensures users can access the right notebook when running the example in Google Colab.
39-39: LGTM: Directory navigation command updated correctly.The directory navigation command has been updated to reflect the new location of the MLOps starter example. This change ensures users navigate to the correct directory when running the example locally.
48-48: LGTM: Notebook opening instruction simplified.The instruction for opening the notebook has been simplified, reflecting the new file structure where the notebook is directly in the current directory. This change makes it easier for users to locate and open the correct notebook file.
tests/integration/examples/bentoml/steps/prediction_service_loader.py (2)
14-14: Imports are appropriate and necessary.The imports of
Unionandcastfrom thetypingmodule are required for the updated return type annotation and type casting within the function.
20-22: Correctly importing service classes.The import statements for
BentoMLContainerDeploymentServiceandBentoMLLocalDeploymentServiceare necessary for type annotations and casting, reflecting the possible return types of the function.src/zenml/integrations/bentoml/steps/bentoml_deployer.py (12)
16-16: Correct inclusion of type annotationsThe addition of
Literalandcastfrom thetypingmodule enhances type checking and code clarity.
25-28: Appropriate import of new deployment classesImporting
BentoMLContainerDeploymentConfig,BentoMLContainerDeploymentService,BentoMLLocalDeploymentConfig, andSSLBentoMLParametersConfigis necessary for handling different deployment types and SSL configurations.
32-32: ImportingBaseServicefor generalized return typeIncluding
BaseServicesupports the updated return type, accommodating multiple service types.
43-43: Introduction ofdeployment_typeparameterAdding
deployment_type: Literal["local", "container"] = "local"allows users to specify the deployment method explicitly. UtilizingLiteralimproves type safety and code readability.
50-52: Addition of container-specific parametersThe new parameters
image,image_tag, andplatformenable customization of the container deployment. Ensure these are properly validated whendeployment_typeis set to"container".Would you like to add validation to ensure these parameters are provided when required for container deployments?
76-78: Updated docstrings for new parametersIncluding documentation for
image,image_tag, andplatformensures that users understand how to utilize these options for container deployments.
114-132: Correct configuration for container deploymentThe configuration for
"container"deployment type is appropriately set up usingBentoMLContainerDeploymentConfig. All necessary parameters are included.
134-159: Maintained configuration for local deploymentThe existing logic for
"local"deployment type remains intact withBentoMLLocalDeploymentConfig. This ensures backward compatibility and consistent behavior.
163-163: Passingservice_typeto service finderIncluding
service_typewhen callingfind_model_serverensures the correct service instances are retrieved based on deployment type.
168-171: Casting services based on deployment typeUsing
castto specify the exact service type enhances type safety and prevents potential runtime errors.
185-204: Deployment logic for new servicesThe deployment logic correctly handles both
"container"and"local"deployment types, ensuring that services are deployed with the appropriate configurations.
61-61: Change of return type toBaseServiceUpdating the return type to
BaseServicereflects that the function can now return different service types. Verify that this change doesn't affect downstream code that expects a specific service subclass.Run the following script to check for potential issues in the codebase:
src/zenml/integrations/bentoml/services/bentoml_local_deployment.py (2)
207-208: 🛠️ Refactor suggestionUse explicit BentoML version check instead of
isinstanceRelying on
isinstance(svc, Service)to determine the BentoML version may not be reliable or future-proof. It's better to explicitly check the BentoML version usingbentoml.__version__.Apply this diff to improve version checking:
+from packaging import version +import bentoml ... - if isinstance(svc, Service): + if version.parse(bentoml.__version__) < version.parse("1.2"):This ensures that the code accurately detects the BentoML version and behaves correctly across different versions.
Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
207-207: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
Line range hint
105-127: Review unused attributesmodel_nameandmodel_uriThe attributes
model_nameandmodel_uriare defined but not utilized in the current implementation. Keeping unused attributes may cause confusion.Run the following script to check for usages of
model_nameandmodel_uri:If these attributes are indeed unused, consider removing them to simplify the configuration.
src/zenml/integrations/bentoml/services/bentoml_local_deployment.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/bentoml/services/bentoml_local_deployment.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/bentoml/model_deployers/bentoml_model_deployer.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/bentoml/model_deployers/bentoml_model_deployer.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/bentoml/services/bentoml_container_deployment.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/bentoml/services/bentoml_container_deployment.py
Outdated
Show resolved
Hide resolved
| @@ -45,34 +45,53 @@ The ZenML integration will provision a local HTTP deployment server as a daemon | |||
|
|
|||
| ## How do you use it? | |||
|
|
|||
| The recommended flow to use the BentoML model deployer is to first [create a BentoML Service](bentoml.md#bentoml-service-and-runner), then [use the `bento_builder_step`](bentoml.md#zenml-bento-builder-step) to build the model and service into a bento bundle, and finally [deploy the bundle with the `bentoml_model_deployer_step`](bentoml.md#zenml-bentoml-deployer-step). | |||
| The recommended flow to use the BentoML model deployer is to first [create a BentoML Service](bentoml.md#create-a-bentoml-service), then [use the `bento_builder_step`](bentoml.md#zenml-bento-builder-step) to build the model and service into a bento bundle, and finally [deploy the bundle with the `bentoml_model_deployer_step`](bentoml.md#zenml-bentoml-deployer-step). | |||
There was a problem hiding this comment.
I think we don't want to recommend using this pre-built-in steps anymore
|
|
||
| ``` | ||
|
|
||
| ### ZenML Bento Builder step | ||
|
|
||
| Once you have your bento service and runner defined, we can use the built-in bento builder step to build the bento bundle that will be used to serve the model. The following example shows how can call the built-in bento builder step within a ZenML pipeline. | ||
| Once you have your bento service defined, we can use the built-in bento builder step to build the bento bundle that will be used to serve the model. The following example shows how can call the built-in bento builder step within a ZenML pipeline. Make sure you have the bento service file in your repository and then use the correct class name in the `service` parameter. |
There was a problem hiding this comment.
I think not only that it should exist in repository but actually in the root otherwise it won't be found
| model = load_artifact_from_response(model) | ||
|
|
||
| # 4. Save the model to a BentoML model based on the model type | ||
| try: | ||
| module = importlib.import_module(f".{model_type}", "bentoml") | ||
| module.save_model(model_name, model, labels=labels) | ||
| except importlib.metadata.PackageNotFoundError: | ||
| bentoml.picklable_model.save_model( | ||
| model_name, | ||
| model, | ||
| ) |
There was a problem hiding this comment.
If we are giving users the power to do this in a step then we should remove the section. in top where we talk about using built-in steps and here actually link doc to how model can be loaded with a BentoML model the official way where they can libraries
| We have now built our bento bundle, and we can use the built-in `bentoml_model_deployer_step` to deploy the bento bundle to our local HTTP server or to a containerized service running in your local machine. | ||
|
|
||
| {% hint style="info" %} | ||
| The `bentoml_model_deployer_step` can only be used in a local environment. But in the case of using containerized deployment, you can use the Docker image created by the `bentoml_model_deployer_step` to deploy your model to a remote environment. It is automatically pushed to your ZenML Stack's container registry. | ||
| {% endhint %} | ||
|
|
||
| **Local deployment** | ||
|
|
||
| The following example shows how to use the `bentoml_model_deployer_step` to deploy the bento bundle to a local HTTP server. | ||
|
|
||
| ```python | ||
| from zenml import pipeline, step | ||
| from zenml.integrations.bentoml.steps import bentoml_model_deployer_step | ||
|
|
||
| @pipeline | ||
| def bento_deployer_pipeline(): | ||
| bento = ... | ||
| deployed_model = bentoml_model_deployer_step( | ||
| bento=bento | ||
| model_name="pytorch_mnist", # Name of the model | ||
| port=3001, # Port to be used by the http server | ||
| ) | ||
| ``` | ||
|
|
||
| **Containerized deployment** | ||
|
|
||
| The following example shows how to use the `bentoml_model_deployer_step` to deploy the bento bundle to a [containerized service](https://docs.bentoml.org/en/latest/guides/containerization.html) running in your local machine. Make sure you have the `docker` CLI installed on your local machine to be able to build an image and deploy the containerized service. |
There was a problem hiding this comment.
I think what we want to give the user the ability to do is bento_deployer_step and not bento_builder_step as that one is straight-forward
| """ | ||
| if ( | ||
| service_instance.SERVICE_TYPE.name | ||
| == "bentoml-container-deployment" |
There was a problem hiding this comment.
Can we make this a constant
| service_instance = cast( | ||
| BentoMLContainerDeploymentService, service_instance | ||
| ) | ||
| elif service_instance.SERVICE_TYPE.name == "bentoml-local-deployment": |
| self.update_status() | ||
| return self.status.state == ServiceState.ACTIVE | ||
|
|
||
| # override the container start method to use the root user |
There was a problem hiding this comment.
Does running this require root user for docker?
| "backlog": backlog, | ||
| } | ||
|
|
||
| if deployment_type == "container": |
There was a problem hiding this comment.
can we create an enum for this and have both local and container maybe at some point we add k8s
Describe changes
get-urlCLI command for models now also outputs the prediction API paths that a user sets in their service.py, making the endpoints more discoverable.bentoctlis deprecated now.Testing
I have tested the container service deployment extensively and used the prediction service deployed locally by it on my machine. I think for the local case I also need to replicate the serve method I have used in the container service but I didn't get the chance to try that yet. I'll be doing that next.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation