Skip to content
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

Upgrade ruff and yamlfix to latest versions before running formatting #2577

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

christianversloot
Copy link
Contributor

Describe changes

When working on #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:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to docs, I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • 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.
  • 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)
  • Other (add details above)

Copy link
Contributor

coderabbitai bot commented Apr 2, 2024

Important

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

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 a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @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.

@strickvl
Copy link
Contributor

strickvl commented Apr 2, 2024

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 strickvl added the enhancement New feature or request label Apr 2, 2024
@christianversloot
Copy link
Contributor Author

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.

@strickvl strickvl added the dependencies Pull requests that update a dependency file label Apr 2, 2024
@avishniakov
Copy link
Contributor

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 🙁

@christianversloot
Copy link
Contributor Author

@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
Copy link
Contributor Author

Made some preliminary changes based on the feedback :)

Copy link
Contributor

@avishniakov avishniakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦭

scripts/format.sh Outdated Show resolved Hide resolved
@christianversloot
Copy link
Contributor Author

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

Copy link
Contributor

@avishniakov avishniakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work 🙂

@strickvl strickvl requested review from bcdurak and removed request for safoinme April 8, 2024 10:57
Copy link
Contributor

@bcdurak bcdurak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @christianversloot,

Thank you for your contribution. This seems like a great addition. I found myself in the same situation that you described a few times in the past, this will be really helpful.

However, I am not a big fan of singling out the installment of these two packages, especially with a --no-deps flag:

  • Even if you remove the --no-deps flag, it might install packages that have a conflicting dependency with the main ZenML installation as @avishniakov pointed out before.
  • If you keep it, it might eventually lead to errors due to the missing dependencies. Moreover, it can also cause an issue if our package is suitable to work with the latest version of yamlfix and ruff due to the other dependencies in the dev extra.

If we assume that the script should only run in an environment where ZenML is already installed, I would instead recommend an approach similar to something like this:

# Get the installed ZenML version
installed_version=$(pip show "zenml" | grep Version | awk '{print $2}')

if [ -n "$install_version"]; then
    pip install uv
    uv pip install "zenml[dev]==$installed_version"
else
    echo "ZenML not installed! Execute 'pip install zenml[dev]' before running the formatting script."
    exit 1
fi

This will fail if zenml is not installed. Timewise, I have tested installing just the pure zenml (without the dev extra) then it takes roughly 3 seconds on my machine. This would happen only once though. If ZenML and the dev extra are already installed, it takes roughly 1.3 seconds on average. (almost the same timing with the current approach)

@christianversloot
Copy link
Contributor Author

@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 :)

@strickvl strickvl requested a review from bcdurak April 9, 2024 08:06
@bcdurak
Copy link
Contributor

bcdurak commented Apr 9, 2024

@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 :)

@christianversloot
Copy link
Contributor Author

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

@avishniakov
Copy link
Contributor

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

@christianversloot
Copy link
Contributor Author

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?

@strickvl
Copy link
Contributor

Agreed

@bcdurak
Copy link
Contributor

bcdurak commented Apr 11, 2024

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

     pip install "zenml[dev]"
     pip install uv 
     uv 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.

@christianversloot
Copy link
Contributor Author

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.

scripts/format.sh Outdated Show resolved Hide resolved
@bcdurak
Copy link
Contributor

bcdurak commented Apr 12, 2024

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.

@christianversloot
Copy link
Contributor Author

Thanks @bcdurak @avishniakov !

Co-authored-by: Andrei Vishniakov <[email protected]>
Copy link
Contributor

@bcdurak bcdurak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦭

@jlopezpena
Copy link
Contributor

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

@zenml-io zenml-io deleted a comment from gitguardian bot Apr 22, 2024
@zenml-io zenml-io deleted a comment from gitguardian bot Apr 30, 2024
@bcdurak
Copy link
Contributor

bcdurak commented Apr 30, 2024

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.

Copy link

gitguardian bot commented May 7, 2024

⚠️ 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 031d3fe src/zenml/cli/init.py View secret
10364910 Triggered Username Password abd943b 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request run-slow-ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants