zenml
zenml copied to clipboard
Upgrade ruff and yamlfix to latest versions before running formatting
Describe changes
When working on https://github.com/zenml-io/zenml/pull/2572 I got a large diff when running scripts/format.sh
because my version of ruff
installed in the environment was quite old.
This PR implements a suggested approach given how ZenML tests are performed these days (at least that's what I learned from @strickvl) - which is to automatically attempt pip install --upgrade
for both ruff
and yamlfix
in scripts/format.sh
, unless the user explicitly chooses not to.
Feel free to decline this PR if you don't think this should be in there, but at least it makes testing a bit more convenient when developing.
Pre-requisites
Please ensure you have done the following:
- [x] I have read the CONTRIBUTING.md document.
- [x] If my change requires a change to docs, I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [x] I have based my new branch on
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop. - [x] If my changes require changes to the dashboard, these changes are communicated/requested.
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [x] Other (add details above)
[!IMPORTANT]
Review skipped
Auto reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the
.coderabbit.yaml
file in this repository. To trigger a single review, invoke the@coderabbitai review
command.You can disable this status message by setting the
reviews.review_status
tofalse
in the CodeRabbit configuration file.
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?
Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
-
I pushed a fix in commit <commit_id>.
-
Generate unit testing code for this file.
-
Open a follow-up GitHub issue for this discussion.
-
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitai
in a new review comment at the desired location with your query. Examples:-
@coderabbitai generate unit testing code for this file.
-
@coderabbitai modularize this function.
-
- PR comments: Tag
@coderabbitai
in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:-
@coderabbitai generate interesting stats about this repository and render them as a table.
-
@coderabbitai show all the console.log statements in this repository.
-
@coderabbitai read src/utils.ts and generate unit testing code.
-
@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
-
@coderabbitai help me debug CodeRabbit configuration file.
-
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)
-
@coderabbitai pause
to pause the reviews on a PR. -
@coderabbitai resume
to resume the paused reviews. -
@coderabbitai review
to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. -
@coderabbitai full review
to do a full review from scratch and review all the files again. -
@coderabbitai summary
to regenerate the summary of the PR. -
@coderabbitai resolve
resolve all the CodeRabbit review comments. -
@coderabbitai configuration
to show the current CodeRabbit configuration for the repository. -
@coderabbitai help
to get help.
Additionally, you can add @coderabbitai ignore
anywhere in the PR description to prevent this PR from being reviewed.
CodeRabbit Configration File (.coderabbit.yaml
)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yaml
file to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
To make this run faster, wondering if it's not time to add uv
to our dev dependencies as well and then doing the install here with uv pip install
. Also / alternatively, wondering whether to switch the flag round, i.e. to have the default not to do the install. Would let others on the team throw in their opinions here :) We'd also want an update to the CONTRIBUTING.md docs about this extra flag, I think.
uv
sounds like a good idea to me. I can also see your point about doing it the other way around; however, I chose to do it this way because forgetting about the upgrades can then never happen. I'm looking forward to the others' opinions and I'll make the necessary changes.
Thanks for contributing @christianversloot !
My take on this:
- I run
format.sh
as on save function in my VSCode, so runningpip install
on every file save isn't very nice to me - On the other hand - it is still fast enough, maybe a couple of seconds to run
- What about just switching it to
uv
here without dev deps? Dev deps can go as some separate flag, but I don't see big benefits of that right away, tbh. @strickvl up to you to advise, cause my experiments with runninguv pip install --upgrade ruff yamlfix
right away ended up in acquiring pydantic v2 immediately 🙁
@avishniakov does your VSCode setup allow for flags? If so, you could set things up e.g. as bash format.sh --no-pip-install
and it should work as it currently does.
Also, do we want to assume that uv
is installed by the user? Or do we want to install it within the script (taking a few extra seconds)? My intuition says the first, and maybe reflect this as a comment in the script e.g. # Assumes uv is installed, if not run pip install uv
.
Made some preliminary changes based on the feedback :)
@avishniakov @strickvl both commits have been pushed, if you'd like the tests can be run again.
@bcdurak I like the suggestion. Some questions in return before I'll adapt
- Is it guaranteed that
zenml[dev]
contains the dependencies that we need here, beingyamlfix
andruff
? - What do you think about adding an
--upgrade
flag to theuv pip install
line? Reason why is that ifzenml[dev]
is already installed, one may still end up with an outdated ruff version this way. - To be 100% sure, I think the
if
statement should check thatinstalled_version
is not empty, notinstall_version
, shouldn't it? Or maybe I'm missing something :)
@christianversloot Those are some good questions. If I go through them one by one:
- Yes. Both
ruff
andyamlfix
are included in our dev dependencies. This is also how we utilize them in our internal development. - This is the one question I am not so sure about.
- On the one hand, I like the idea because it keeps everything fresh and updated without leaving the boundaries of the ZenML installation.
- However, on the other hand, it might upgrade some packages that were originally required by ZenML, which might eventually change the environment people are developing in. Also, in such cases, it would take a few more seconds to run.
- Adding it might actually be a blessing in disguise because a change in the environment would only occur if a new version of a dependency is released. In such cases, when you are developing, it is a nice advantage to be in the most up-to-date state as possible.
- Right now, I am more in favor of adding it but maybe we can ask @avishniakov's opinion on this as well.
- Lastly, you are right, that's a typo from my side when I was sketching it. Sorry about the confusion :)
Thanks for your response @bcdurak. I'll await @avishniakov's as well before making changes that turn out fruitless.
@bcdurak, I'd prefer to keep it as simple as possible - this is just a formatted script, no heavy lifting here, so I'd say - go for just ruff
and yamlfix
as it was and not deal with zenml[dev]
, as it might indeed change the env, while the chances of env being heavily impacted by ruff
and/or yamlfix
is quite low, IMO. Let's remove --no-deps
flag, since it is the same as zenml[dev]
, right? But a bit smaller impact.
I can offer another option (more complicated to code, though): check which ruff
and yamlfix
are installed, if outdated - give a confirmation request asking that the upgrade will happen via uv pip...
.
Thanks @avishniakov! We could choose to keep things as they are atm and then make the more complex changes should they be necessary in the future? WDYT @strickvl @bcdurak?
Agreed
@avishniakov @strickvl The more I think about this, I favor my previous suggestion a bit more.
-
For your first point, I would argue that it is more beneficial for anyone using a "development" version of ZenML that they are as close to the most up-to-date environment as possible. Otherwise, you would be developing in a different environment than people who just did "pip install zenml". This forces the developer's hand to stay up to date with the requirements of the current ZenML package. Plus, once it is set up, it basically takes the same amount of time. There is also an option to opt out of it for any developer who wants to disable the feature.
-
Secondly, you can not keep it as it is and remove the
--no-deps
, right? This is simply the same as doing:ip install "zenml[dev]" ip install uv v pip install --upgrade ruff yamlfix
which ends up installing pydantic>2.0.
-
As for the last option, I initially thought about this as well. However, due to our other dependencies, a developer might end up with a
ruff
andyamlfix
version which is not the max version when they dopip install "zenml[dev]"
. This might end up breaking some dependencies.
Since what looked like a simple change has eaten more time than anticipated, I unfortunately need to withdraw my contribution to this PR, as for me, this is a nice to have. However, do feel free to proceed on your own and if not feel free to close the PR.
Hey @christianversloot,
@avishniakov and I just had a discussion about this and ended up implementing a new solution.
In short, it will give you a warning if your current ruff
and yamlfix
installations do not match the requirements of the ZenML dev package.
Thank you for your contribution on this, it will be very helpful going forward.
Thanks @bcdurak @avishniakov !
Won't installing/changing dependencies on the virtual environment potentially mess up the user's setup? Mixing dependencies installed with poetry
(the system that zenml
uses) and pip/uv in the same environment is generally a bad idea
Hey @jlopezpena, that's a valid concern.
However, this PR only changes the formatting script and thus, does not affect the users but the developers IMO. I would argue that it is more important for the developers to stay up-to-date with the latest usable versions of ruff
and yamlfix
in order to conduct a proper merge. It is also possible to disable this behavior by setting the SKIP_UPGRADE
to True.
⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.
Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.
🔎 Detected hardcoded secrets in your pull request
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
10364910 | Triggered | Username Password | 031d3fe2b023cfa9e10ee858354c7dca2f7ad9f0 | src/zenml/cli/init.py | View secret |
10364910 | Triggered | Username Password | abd943b0ff0a5afa5cd322e7f51c007457eff440 | src/zenml/cli/init.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.