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

installation: Switch to UV for dependency management #3294

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

EdmundGoodman
Copy link
Collaborator

@EdmundGoodman EdmundGoodman commented Oct 12, 2024

Summary

Adopt uv for dependency management, replacing the existing pip approach.

Motivation

uv is developed by Astral, who also wrote ruff which is already in use in xDSL CI pipelines. It offers the benefits of much faster dependency resolution and installation, along with leveraging the more modern pyproject.toml configuration format defined in PEPs 518 and 618.

Requested by @superlopuh

Changes

This replacement requires the following changes:

  • Modification to the venv target of the Makefile to create the virtual environment
  • Update CI flows to install with uv as opposed to pip, including testing
    • ci-core.yaml
    • ci-mlir.yaml
    • ci-notebooks.yaml
    • ci-pyright-fails.yaml
    • code-formatting.yaml
    • jupyterlite.yaml (cannot easily be spawned due to workflow triggers)
    • pythonpublish.yaml (cannot easily be spawned due to required secrets)
    • release-notes.yaml (requires no changes)
  • Update documentation to reflect these changes
  • Test across operating systems
    • Windows
    • MacOS
    • NixOS
  • Resolve changes required to Dependabot

Post-merge checklist

Once merged, we should check the following things:

  • The release flow worked correctly (including versioneer)
  • Users are able to switch to uv and instructions are clear
  • The update-bot workflow creates helpful and correct PRs in a timely manner

Copy link

codecov bot commented Oct 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.11%. Comparing base (f2ba5f7) to head (9b7493a).
Report is 63 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3294      +/-   ##
==========================================
+ Coverage   90.01%   90.11%   +0.10%     
==========================================
  Files         445      450       +5     
  Lines       55850    56792     +942     
  Branches     5372     5478     +106     
==========================================
+ Hits        50272    51177     +905     
- Misses       4169     4172       +3     
- Partials     1409     1443      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

Could you please also migrate the makefile? Maybe with a phony target that checks whether uv is installed, and directs developers to installation instructions. The goal would be for make tests to be runnable outside of an activated venv

@EdmundGoodman EdmundGoodman marked this pull request as draft October 12, 2024 17:46
Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR!
A few quick questions/requests I have:

  • Can we move the removed requirements.txt in the pyproject.toml? That way we can still track them with dependabot, and also this is the only change that's missing to still support virtualenv. I'd personally would like to still continue supporting venv if possible, as for now it is still way more popular than uv.
  • Do you think we can automate the update of uv.lock with dependabot? Currently, we get daily automated updates of dependencies with dependabot, and it would quite annoying to have to update the uv.lock manually every time.
  • We will probably have to test pythonpublish after the PR gets merged. Just ping me and I'll try to run it to see if it works or not, but looking at it this should be fine I think.

@@ -1 +0,0 @@
-e .[gui,jax,riscv,wgpu,onnx,dev]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these dependencies moved to uv.lock?
I feel that this should rather go to the .toml file so that we can track them with dependabot, it was also bad before when they were here, but at least we had a list of them.

Copy link
Member

Choose a reason for hiding this comment

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

No, these are in the pyproject.toml, and are installed via --all-extras

Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

We've been using uv for our quantum stack recently and it seems to work nicely. I have a couple of comments:

  • When to use uv run. It seems in the test ci you are activating the venv anyway so I don't see the reason to us uv run everywhere.
  • If you did want to use uv run everywhere, you would need to make the filecheck tests use it (i.e. every xdsl-opt should be uv run xdsl-opt). You can get most of these by modifying lit.cfg. I'm not sure if this is actually worth it?
  • I've always had problems with the wgpu package and had to manually remove it. Will there be an easy way to do that with the new set up?
  • uv should be added to flake.nix (I can do this afterwards if necessary)

@EdmundGoodman
Copy link
Collaborator Author

EdmundGoodman commented Oct 16, 2024

Ok - I think I see the problem. All the make targets using uv need to have the uv-installed dependency, not just make venv.

I assumed that people would be coming in without having cloned it already, make their venv (and install uv then), then do the other targets. In your case, you already have a venv so went straight to the other targets and it failed.

I will update the PR tonight when I'm back from a commitment to hopefully fix this.

@EdmundGoodman
Copy link
Collaborator Author

Now the Makefile stuff is hopefully fixed, I've also hacked together something which might be able to be a stopgap for Dependabot until it officially supports uv.

I've made a demo repo here: https://github.com/EdmundGoodman/update-bot, but the idea is a cron job which runs every week and creates a PR to bump the uv lockfile. This slightly differs from Dependabot in it is scheduled and does all version bumps not just security ones, but might give enough functionality for now?

If it seems reasonable, the change will just be adding that workflow YAML file to this PR

@superlopuh
Copy link
Member

This is beautiful. If I understand you correctly, the idea would be to loosen the dependabot requirements, and update the dev versions in the lockfile once a week? My understanding is that we are already too fine-grained in our dependency constraints.

Makefile Outdated Show resolved Hide resolved
@EdmundGoodman
Copy link
Collaborator Author

This is beautiful.

😃 ! I have added the new workflow to the PR now

If I understand you correctly, the idea would be to loosen the dependabot requirements, and update the dev versions in the lockfile once a week?

I think Dependabot doesn't support uv at all currently, so we would turn it off (now in the PR), then just bump any dependencies with new versions weekly by merging the automatically generated PR if it passes all the tests.

My understanding is that we are already too fine-grained in our dependency constraints.

I guess some things like filecheck==1.0.1 would need to be modified to be filecheck>=1.0.1 as otherwise uv alone wouldn't bump them even if they are known to require a security patch, yes

Copy link
Member

Choose a reason for hiding this comment

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

what is the impact of this change?

Copy link
Member

Choose a reason for hiding this comment

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

Should we just delete it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dependabot will no longer run - as it would do nothing anyway or throw an error as the package ecosystem is no longer pip as it expects.

An approximation to the behaviour of Dependabot is then supported by the new custom update bot GitHub Action workflow.

When Dependabot (hopefully) eventually supports uv, it can be reverted and the package ecosystem changed to uv

If not disabling Dependabot is a strong constraint (which is fairly reasonable), then moving to uv is blocked till it offers support for it

Copy link
Collaborator Author

@EdmundGoodman EdmundGoodman Oct 26, 2024

Choose a reason for hiding this comment

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

Oops, didn't see the follow up comment - this is would make it marginally easier to (remember to?) revert it when support is added, but realistically git revert would make more sense. I'll just delete it

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm pretty sure I'll see on either the twitter or GH when they enable Dependabot for UV, so I don't think it's a big risk.

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

What's left on the TODO list to open it up for review?

@EdmundGoodman
Copy link
Collaborator Author

EdmundGoodman commented Oct 28, 2024

What's left on the TODO list to open it up for review?

I think mostly just the stuff in the top comment:

  • Testing against non-MacOS operating systems (I will need help with this)
  • Checking the release flow/hard-to-run pipelines won't explode because of the changes (I will need help with this)
  • Checking the update-bot workflow crontab schedules correctly in this instance (I am doing this now -- not listed in the top comment) all looks ok

It may also be worth ensuring there is buy-in from maintainers and key users before what is a fairly impactful if not large change.

@alexarice
Copy link
Collaborator

Hi, I added a commit to update the nix flake, hope that is ok. make tests works for me locally

@superlopuh
Copy link
Member

This LGTM, I guess Mathieu will test the release situation once this is merged

@EdmundGoodman
Copy link
Collaborator Author

Hi, I added a commit to update the nix flake, hope that is ok. make tests works for me locally

Amazing, thanks!

@EdmundGoodman
Copy link
Collaborator Author

EdmundGoodman commented Oct 29, 2024

This LGTM, I guess Mathieu will test the release situation once this is merged

Sounds reasonable - if anything explodes for users when it gets merged feel free to ping me here/on Zulip and I can try to help them debug things (but hopefully everything will go smoothly 😄)

@superlopuh superlopuh marked this pull request as ready for review October 29, 2024 16:31
Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Almost good for me, I just want to be sure we are not regressing too much with dependabot, and keep the documentaiton for doing a pip install.

I'll check the release once it's merged, hopefully it won't break!

Comment on lines 1 to 19
# To get started with Dependabot version updates, you'll need to specify which
# package ecosystems to update and where the package manifests are located.
# Please see the documentation for all configuration options:
# https://help.github.com/github/administering-a-repository/configuration-options-for-dependency-updates

version: 2
updates:
- package-ecosystem: "pip" # See documentation for possible values
directory: "/" # Location of package manifests
schedule:
interval: "daily"
# Add assignees
assignees:
- "math-fehr"
- "georgebisbas"
commit-message:
prefix: "pip prod"
prefix-development: "pip dev"
include: "scope"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we actually keep dependabot, though maybe making the schedule to "weekly" instead?

I feel that update-bot is not as strong as dependabot, as it will not update the actual version, it will just update the minor ones (and miss security updates).
Having both at the same time is probably what we want here.

Copy link
Collaborator Author

@EdmundGoodman EdmundGoodman Oct 31, 2024

Choose a reason for hiding this comment

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

tl;dr, I've just now caught up with you in realising it might be possible to use Dependabot in this case despite the docs saying its not supported. I'll try to make changes to go back to just using Dependabot and see if something breaks

Long chain of thought to justify this to myself when I come back to it tomorrow:

I've just had a further stare at this and am wondering if Dependabot might just work for uv despite not being explicitly supported?

On modern python projects the requirements are often loosely defined in the pyproject.toml file (e.g. package = ^1.2.1), then the specific versions set in a lock file such as Pipenv.lock/poetry.lock/uv.lock - then Dependabot acts on this lock file. Since uv isn't a supported lock file type, Dependabot can't be used like this.

However, I've just now realised that since this project is pinning dependency versions in pyproject.toml (as pip doesn't have a lockfile), Dependabot acts on the pyproject.toml directly -- which I hadn't seen before so didn't think to check. As such if we either: don't commit the uv lock file; or use Dependabot in conjunction with update-bot to bump the lock file, it should all work correctly

In future, it might be worth considering using a lock file instead of pinning dependencies in pyproject.toml -- but for now I think it saves us from this problem so we can just leave it.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation Related to package installation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants