-
Notifications
You must be signed in to change notification settings - Fork 465
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
Catch missing jupyter installation for zenml go
#2571
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe updates primarily focus on enhancing user experience and functionality around running Jupyter notebooks with ZenML. A notable addition is the optional step for installing the Changes
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
E2E template updates in |
@coderabbitai review |
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: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- README.md (1 hunks)
- src/zenml/cli/base.py (2 hunks)
- src/zenml/cli/utils.py (7 hunks)
Additional Context Used
Path-based Instructions (2)
src/zenml/cli/base.py (1)
src/zenml/**/*.py
:
Review the Python code for conformity with Python best practices.src/zenml/cli/utils.py (1)
src/zenml/**/*.py
:
Review the Python code for conformity with Python best practices.
Additional comments not posted (2)
src/zenml/cli/base.py (1)
484-501
: The addition of a pre-emptive check for Jupyter installation in thego
function is a thoughtful usability improvement. It ensures users are informed about the missing Jupyter Notebook or JupyterLab installation before attempting to run the tutorial notebooks, providing a better user experience.src/zenml/cli/utils.py (1)
2670-2683
: The implementation of theis_jupyter_installed
function correctly checks for the presence of thenotebook
package, which is a common way to verify if Jupyter notebooks are installed. However, it's worth noting that JupyterLab, a next-generation web-based user interface for Project Jupyter, might also be used by some users in place of the classic Jupyter notebooks. While JupyterLab can run notebooks without thenotebook
package, it's a common scenario where users have both installed. Therefore, this check is generally sufficient but might not cover all Jupyter-related use cases.
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.
Looks good.
A few small notes (feel free to ignore):
- Do you think that the version of the
notebook
package could change anything here? - I like how you added a comment to the README, it might be nice to do the same to the docs if we have it anywhere.
- Just out of curiosity, did you do the formatting manually?
No I think jupyter takes pretty good care with backwards compatibility. I don't think we need to specify a specific version.
I'll take a look to see where is appropriate.
Not sure what you mean here. |
Quickstart template updates in |
LLM Finetuning template updates in |
…ing-jupyter-zenml-go
…enml-io/zenml into feature/catch-missing-jupyter-zenml-go
LLM Finetuning template updates in |
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.
Tiny nit
I added a check to catch when someone runs
zenml go
and they don't have Jupyter notebooks installed.Summary by CodeRabbit
Documentation
New Features
Refactor