-
Notifications
You must be signed in to change notification settings - Fork 478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modal Step Operator #2948
Modal Step Operator #2948
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📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new integration for the Modal platform within the ZenML framework. This includes the addition of several modules and classes that facilitate the configuration and execution of tasks on Modal's infrastructure. Key components include the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZenML
participant ModalIntegration
participant Modal
User->>ZenML: Initiates workflow with Modal integration
ZenML->>ModalIntegration: Configures integration settings
ModalIntegration->>Modal: Validates required settings
Modal->>ZenML: Returns configuration status
ZenML->>User: Workflow execution begins
ZenML->>Modal: Launches step on Modal
Modal-->>ZenML: Executes step and returns results
ZenML->>User: Delivers results of the workflow
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
|
@coderabbitai review |
Actions performedReview triggered.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files selected for processing (8)
Tip You can disable poems in the walkthrough.Disable the 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 as PR comments)
Additionally, you can add Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
src/zenml/integrations/modal/step_operators/modal_step_operator.py (1)
112-208
: Consider adding more comments for clarity.The
launch
method is detailed and includes error handling, but additional comments could improve readability and maintainability.Consider adding comments to explain the purpose of each major block of code within the method.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- docs/mocked_libs.json (1 hunks)
- src/zenml/integrations/init.py (1 hunks)
- src/zenml/integrations/constants.py (1 hunks)
- src/zenml/integrations/modal/init.py (1 hunks)
- src/zenml/integrations/modal/flavors/init.py (1 hunks)
- src/zenml/integrations/modal/flavors/modal_step_operator_flavor.py (1 hunks)
- src/zenml/integrations/modal/step_operators/init.py (1 hunks)
- src/zenml/integrations/modal/step_operators/modal_step_operator.py (1 hunks)
Files skipped from review due to trivial changes (2)
- src/zenml/integrations/constants.py
- src/zenml/integrations/modal/step_operators/init.py
Additional context used
Path-based instructions (5)
src/zenml/integrations/modal/flavors/__init__.py (1)
Pattern
src/zenml/**/*.py
: Review the Python code for conformity with Python best practices.src/zenml/integrations/modal/__init__.py (1)
Pattern
src/zenml/**/*.py
: Review the Python code for conformity with Python best practices.src/zenml/integrations/__init__.py (1)
Pattern
src/zenml/**/*.py
: Review the Python code for conformity with Python best practices.src/zenml/integrations/modal/flavors/modal_step_operator_flavor.py (1)
Pattern
src/zenml/**/*.py
: Review the Python code for conformity with Python best practices.src/zenml/integrations/modal/step_operators/modal_step_operator.py (1)
Pattern
src/zenml/**/*.py
: Review the Python code for conformity with Python best practices.
Additional comments not posted (12)
src/zenml/integrations/modal/flavors/__init__.py (2)
1-14
: LGTM!The license header and docstring are correctly formatted.
The code changes are approved.
16-26
: LGTM!The import statements and
__all__
declaration are correctly implemented.The code changes are approved.
src/zenml/integrations/modal/__init__.py (4)
1-18
: LGTM!The license header and docstring are correctly formatted.
The code changes are approved.
19-25
: LGTM!The import statements and constant declaration are correctly implemented.
The code changes are approved.
28-43
: LGTM!The class definition and methods are correctly implemented.
The code changes are approved.
46-46
: LGTM!The installation check is correctly implemented.
The code changes are approved.
src/zenml/integrations/__init__.py (1)
49-49
: LGTM!The import statement for
ModalIntegration
is correctly implemented.The code changes are approved.
src/zenml/integrations/modal/flavors/modal_step_operator_flavor.py (3)
26-51
: LGTM!The
ModalStepOperatorSettings
class is well-defined with comprehensive docstrings. The attributes cover essential configurations for the Modal step operator.The code changes are approved.
54-70
: LGTM!The
ModalStepOperatorConfig
class is well-structured and theis_remote
property is correctly implemented. The docstring provides clear explanations.The code changes are approved.
73-130
: LGTM!The
ModalStepOperatorFlavor
class is well-implemented with correctly defined properties and informative docstrings. The class follows Python best practices.The code changes are approved.
docs/mocked_libs.json (1)
254-257
: LGTM!The additions of
modal
,modal.cli
, andmodal.cli.run
expand the library's capabilities. The overall structure of the JSON remains unchanged.The code changes are approved.
src/zenml/integrations/modal/step_operators/modal_step_operator.py (1)
44-49
: LGTM!The
ModalStepOperator
class is well-implemented with a clear purpose and structure. The docstring provides a good overview.The code changes are approved.
src/zenml/integrations/modal/step_operators/modal_step_operator.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/modal/flavors/modal_step_operator_flavor.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/modal/step_operators/modal_step_operator.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/modal/step_operators/modal_step_operator.py
Outdated
Show resolved
Hide resolved
src/zenml/integrations/modal/step_operators/modal_step_operator.py
Outdated
Show resolved
Hide resolved
Some answers for your general questions/comments:
I would suggest the following:
|
Ah yes you're right. Actually because of the way their CLI works (and the fact that we're now using subprocess), it's even a bit harder to get the URL for the specific run. Since we don't yet have it for other step operators, I think we can just leave it for now maybe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a tiny nit which can be ignored
…ml into feature/modal-step-operator
…ml into feature/modal-step-operator
LLM Finetuning template updates in |
Classification template updates in |
E2E template updates in |
Seeking an earlyish review before I write the docs (in case anything major changes). This probably isn't the most elegant way of doing this, but it works at least. It supports both CPU and GPU-based workloads, and specifying CPU and memory for the instance on which the step will run.
See Demo tenant for the run details. Screenshot from Modal dashboard:
A simple dev pipeline to test this:
Remaining tasks
Summary by CodeRabbit
New Features
Bug Fixes
Documentation