-
Notifications
You must be signed in to change notification settings - Fork 436
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
Upgrade ruff and yamlfix to latest versions before running formatting #2577
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 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 (
|
To make this run faster, wondering if it's not time to add |
|
Thanks for contributing @christianversloot ! My take on this:
|
@avishniakov does your VSCode setup allow for flags? If so, you could set things up e.g. as Also, do we want to assume that |
Made some preliminary changes based on the feedback :) |
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.
🦭
Co-authored-by: Andrei Vishniakov <31008759+avishniakov@users.noreply.github.com>
@avishniakov @strickvl both commits have been pushed, if you'd like the tests can be run again. |
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.
Thanks for your work 🙂
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.
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
andruff
due to the other dependencies in thedev
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)
@bcdurak I like the suggestion. Some questions in return before I'll adapt
|
@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 I can offer another option (more complicated to code, though): check which |
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.
|
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 Thank you for your contribution on this, it will be very helpful going forward. |
Thanks @bcdurak @avishniakov ! |
Co-authored-by: Andrei Vishniakov <31008759+avishniakov@users.noreply.github.com>
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.
🦭
Won't installing/changing dependencies on the virtual environment potentially mess up the user's setup? Mixing dependencies installed with |
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 |
|
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
- 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.
Describe changes
When working on #2572 I got a large diff when running
scripts/format.sh
because my version ofruff
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 bothruff
andyamlfix
inscripts/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:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes