zenml icon indicating copy to clipboard operation
zenml copied to clipboard

Upgrade ruff and yamlfix to latest versions before running formatting

Open christianversloot opened this issue 10 months ago • 20 comments

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 targeting develop. 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)

christianversloot avatar Apr 02 '24 10:04 christianversloot

[!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 to false 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?

Share
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.

coderabbitai[bot] avatar Apr 02 '24 10:04 coderabbitai[bot]

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.

strickvl avatar Apr 02 '24 10:04 strickvl

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.

christianversloot avatar Apr 02 '24 11:04 christianversloot

Thanks for contributing @christianversloot !

My take on this:

  • I run format.sh as on save function in my VSCode, so running pip 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 running uv pip install --upgrade ruff yamlfix right away ended up in acquiring pydantic v2 immediately 🙁

avishniakov avatar Apr 04 '24 07:04 avishniakov

@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.

christianversloot avatar Apr 05 '24 08:04 christianversloot

Made some preliminary changes based on the feedback :)

christianversloot avatar Apr 05 '24 08:04 christianversloot

@avishniakov @strickvl both commits have been pushed, if you'd like the tests can be run again.

christianversloot avatar Apr 08 '24 08:04 christianversloot

@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, being yamlfix and ruff?
  • What do you think about adding an --upgrade flag to the uv pip install line? Reason why is that if zenml[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 that installed_version is not empty, not install_version, shouldn't it? Or maybe I'm missing something :)

christianversloot avatar Apr 08 '24 14:04 christianversloot

@christianversloot Those are some good questions. If I go through them one by one:

  • Yes. Both ruff and yamlfix 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 :)

bcdurak avatar Apr 09 '24 12:04 bcdurak

Thanks for your response @bcdurak. I'll await @avishniakov's as well before making changes that turn out fruitless.

christianversloot avatar Apr 09 '24 13:04 christianversloot

@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....

avishniakov avatar Apr 09 '24 14:04 avishniakov

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?

christianversloot avatar Apr 11 '24 08:04 christianversloot

Agreed

strickvl avatar Apr 11 '24 08:04 strickvl

@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 and yamlfix version which is not the max version when they do pip install "zenml[dev]". This might end up breaking some dependencies.

bcdurak avatar Apr 11 '24 13:04 bcdurak

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.

christianversloot avatar Apr 11 '24 15:04 christianversloot

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.

bcdurak avatar Apr 12 '24 08:04 bcdurak

Thanks @bcdurak @avishniakov !

christianversloot avatar Apr 12 '24 15:04 christianversloot

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

jlopezpena avatar Apr 17 '24 09:04 jlopezpena

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.

bcdurak avatar Apr 30 '24 14:04 bcdurak

⚠️ 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
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. 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


🦉 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.

gitguardian[bot] avatar May 07 '24 18:05 gitguardian[bot]