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

Support uv compiled requirements files #10040

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

Conversation

avilaton
Copy link

@avilaton avilaton commented Jun 19, 2024

What are you trying to accomplish?

Trying to draft support for using https://github.com/astral-sh/uv as a replacement for pip-tools in dependabot. The reason for this is that uv is much faster and many projects have already started switching to it. UV is a pip-tools compatible replacement written in rust.

This is a proposal for:

Anything you want to highlight for special attention from reviewers?

Even if uv isn't adopted right now, it might be the long term solutions for pip-compile slowness. We use it for generating requirements.txt and each time dependabot does it with pip-compile we get a slightly different output which we later correct manually.

How will you know you've accomplished your goal?

The change I introduced is fairly simple. First I look at the requirements file header to identify if uv was used to generate it. If it was, I change the command used from pyenv exec pip-compile to pyenv exec uv pip compile.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@avilaton avilaton changed the title Patch 1 Support python uv as pip-compile compatible replacement Jun 19, 2024
@mgaitan
Copy link

mgaitan commented Jul 11, 2024

we at @Shiphero are looking forward to using this functionality as soon as it becomes available. Currently we use dependabot only as an alert to recompile manually as we have to manage everything via uv (and it generates slightly different results than pip-compile).

@mgaitan
Copy link

mgaitan commented Aug 12, 2024

@edgarrmondragon what is missing to merge this? it is a very valuable feature.

avilaton and others added 7 commits August 22, 2024 17:09
Co-authored-by: Edgar Ramírez Mondragón <16805946+edgarrmondragon@users.noreply.github.com>
Co-authored-by: skshetry <18718008+skshetry@users.noreply.github.com>
Co-authored-by: Edgar Ramírez Mondragón <16805946+edgarrmondragon@users.noreply.github.com>
@avilaton
Copy link
Author

@edgarrmondragon I think we are in a better position now

@edgarrmondragon
Copy link

@edgarrmondragon I think we are in a better position now

Indeed. Would love to get some response here from the maintainers.

Maybe we also need an update to https://github.com/dependabot/smoke-tests coupled with this?

@ssbarnea
Copy link

@robaiken @sachin-sandhu Any chance you could look at this one, please?

@skshetry
Copy link

skshetry commented Sep 2, 2024

I think we should also add a test for --universal flag, which performs a universal resolution - so that it's supported right away.

@Mogost
Copy link

Mogost commented Sep 2, 2024

First of all - uv is awesome and I am 100% pro adding uv support in dependabot.
However, I have concerns that this pull request might not be heading in the right direction. uv basically gives compatibility mode, but there are some differences like unsafe packages at the end of the file. Moreover, pip-tools is more used so far, and it is probably better to use it so that the result for people using pip-tools is not different from dependabot.
Any fixes from pip-tools team will no longer make sense after this PR.
At the same time, the uv team made their own .lock file, which seems right to me. It looks like it would be more appropriate to prioritize work on adding uv support instead of replacing pip-tools.
This pull request is severely lacking in the dependabot maintainers to direct the work in the right direction.

@sachin-sandhu @jeffwidman @robaiken Please help channel the community's efforts

@jonjanego
Copy link
Member

Hi all, I'm one of the product managers at GitHub responsible for Dependabot.

First off, thank you all for the great collaboration and discussion in here, and especially from @avilaton for taking the initiative to start this work and the discussion.

We discussed this internally as a GitHub team and came to effectively the same conclusion as @Mogost in this comment: while uv is certainly worth us looking into supporting, using it as a full-scale replacement for what we are doing with how Dependabot uses pip is likely to be problematic in a variety of respects. Instead, I suggest that this PR be pivoted in the direction of supporting uv as a standalone ecosystem. This also appears to be in line with the uv team's goal of how uv is intended to be used, as a "drop-in replacement" - yet with its own set of commands.

I hope that this helps clarify what we think would be best for Dependabot, and sorry for the lack of attention. in the future please don't hesitate to ping me for any ecosystem / coverage related suggestions for Dependabot.

@beagold
Copy link

beagold commented Sep 4, 2024

Hey @jonjanego and @Mogost!

Thanks for both your comments!

Just wanted to chime in because I have a feeling from both your comments you might have misunderstood the pull request (or I might be the one misunderstanding the code changes)

This pr doesn't replace pip-tools in favour of uv. I don't think anyone would appreciate that as it would just cause issues with people currently using pip-tools. This pr seems to just add support for uv compiled dependency files along side pip-tools, so in the case it is detected to have been generated using uv by using the header, then it is regenerated using uv instead of pip-compile.

I personally think that's a reasonable implementation and one that could be welcome into dependabot without breaking any existing functionality :)

EDIT: Forgot to mention that this is most probably due to how the initial pull request comment was worded, so just wanted to explain a bit what the pr actually achieves


options << "--no-annotate" unless requirements_file.content.include?("# via ")

options << "--no-header"
Copy link

Choose a reason for hiding this comment

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

This line slightly confuses me, as wouldn't this remove the only marker that makes it possible to identify uv compiled locks?

Copy link

Choose a reason for hiding this comment

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

Is it possible to specifically request uv instead of pip-tools when no header is present?

Copy link

Choose a reason for hiding this comment

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

Dependabot currently doesn't allow specifying what tooling to use for compiled dependencies to my knowledge, it just has been pip-compile for pip resolver, always.

Maybe in the future, configuration could be added for it, but I don't think this is the case, so keeping the header is most probably something that is wanted. Otherwise, as the current code stands, I don't think it will even be able to pickup that it's a dependency compilation file (even for pip-compile)

@albertferras-vrf
Copy link

This pr seems to just add support for uv lock files along side pip-tools, so in the case that a lockfile is detected to have been generated using uv by using the header, then it is regenerated using uv instead of pip-compile.

edit: The first time I read this I understood you were refering to uv.lock files, which this PR is not generating. It does update the lock (.txt) files with uv pip compile instead of pip compile 👍

uv can generate lock files with uv pip compile (updating requirements.txt) or uv sync (updating/generating uv.lock). I think it makes sense that dependabot has support for both, and this PR is one part of it

@Mogost
Copy link

Mogost commented Sep 4, 2024

After your clarification, it makes more sense to me. What is really going on here is checking whether the file was generated with uv or with pip-tools. So it really makes sense.
i.e. this PR introduces support for uv in pip-tools compatibility mode, without removing pip-tools support as it is. the next PR after this PR should be a PR with support for the .lock file of uv itself. So we get pip-tools support, uv (compatibility mode), uv (native mode).

Also uv team discus dependabot support here astral-sh/uv#2512
I just want to summon here @zanieb, because I think that community effort should be directed by dependabot product team (now we have here @jonjanego) and uv team (where I belive Zanie will be the best candidate).

This part of the code is crucial and I missed it the first time I looked at it
https://github.com/avilaton/dependabot-core/blob/1c081bf4bd3e9c7a97f305920e36e40f799a72ef/python/lib/dependabot/python/update_checker/pip_compile_version_resolver.rb#L254-L261

@zanieb
Copy link

zanieb commented Sep 4, 2024

This makes sense to me.

As a minor note, we allow exporting the uv.lock file to a requirements.txt file and some people want to commit both (astral-sh/uv-pre-commit#16). When adding uv.lock support in the future, you'll probably need to ignore or also update exported requirements.txt files which will have the header:

# This file was autogenerated via `uv export`.

Let me know if there are any specific questions here.

@avilaton
Copy link
Author

avilaton commented Sep 8, 2024

Hey @jonjanego and @Mogost!

Thanks for both your comments!

Just wanted to chime in because I have a feeling from both your comments you might have misunderstood the pull request (or I might be the one misunderstanding the code changes)

This pr doesn't replace pip-tools in favour of uv. I don't think anyone would appreciate that as it would just cause issues with people currently using pip-tools. This pr seems to just add support for uv compiled dependency files along side pip-tools, so in the case it is detected to have been generated using uv by using the header, then it is regenerated using uv instead of pip-compile.

I personally think that's a reasonable implementation and one that could be welcome into dependabot without breaking any existing functionality :)

EDIT: Forgot to mention that this is most probably due to how the initial pull request comment was worded, so just wanted to explain a bit what the pr actually achieves

Help me out here please, I think you are correct about the title of the PR being misleading. I think what I'm proposing is more like "Use uv for requirements.txt files if present in their header". Will that help?

@beagold
Copy link

beagold commented Sep 9, 2024

Help me out here please, I think you are correct about the title of the PR being misleading. I think what I'm proposing is more like "Use uv for requirements.txt files if present in their header". Will that help?

@avilaton I would personally use "Support uv compiled requirements files". Not mentioning replacing pip-compile in the title would probably be best.

@beagold
Copy link

beagold commented Sep 9, 2024

Also, would be cool if you could have a look at the review I made :)

@avilaton avilaton changed the title Support python uv as pip-compile compatible replacement Support uv compiled requirements files Sep 9, 2024
…ater.rb

Co-authored-by: beagold <86345081+beagold@users.noreply.github.com>
…ater.rb

Co-authored-by: beagold <86345081+beagold@users.noreply.github.com>
@jonjanego
Copy link
Member

jonjanego commented Sep 10, 2024

Hi everyone. Thanks for clarifying the intentions here and continuing to collaborate! When this PR gets into a state that you think is close to merge-able, please tag me and I'll work with our engineering team to get someone to take a look so we can get this over the line.

Also, if someone contributing here could also open a corresponding PR to the Dependabot documentation to suggest docs changes to support this improvement, it would be greatly appreciated, as doing that would be another requirement before we could merge this in. Please link to that PR when it's ready.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.