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 17 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).

@avilaton avilaton force-pushed the patch-1 branch 7 times, most recently from 915a9ec to 58f992e Compare July 14, 2024 01:05
@avilaton avilaton marked this pull request as ready for review July 14, 2024 01:07
@avilaton avilaton requested a review from a team as a code owner July 14, 2024 01:07
@mgaitan
Copy link

mgaitan commented Aug 12, 2024

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

@avilaton
Copy link
Author

Hi @avilaton , thank you so much for the dev work, really appreciate it !!

I'm one of devs working on dependabot I'm taking a look at it to make sure everything is good to go. Will come back with more info if needed.

First time I contribute here so it is very likely we are missing something. Edit as you see fit, I just thought this was a good way to get the ball rolling.

options.join(" ")
if (requirements_file = compiled_file_for_filename(filename))
if requirements_file.content.include?("uv pip compile")
options += uv_pip_compile_options_from_compiled_file(requirements_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @avilaton , looks like you added 2 new functions uv_pip_compile_options_from_compiled_file , pip_compile_options_from_compiled_file in pip_compile_version_resolver.rb, i think they are being referenced from pip_compile_file_updater.rb can you please fix this (IDE is showing it as no function exists in Dependabot::Python::UpdateChecker::PipCompileVersionResolver)

Copy link
Author

Choose a reason for hiding this comment

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

Would be neat to have a way not to repeat this code, it seems to me as if it would be more resilient to have them centralized. What do you suggest we should do?

Copy link
Contributor

Choose a reason for hiding this comment

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

@avilaton , if you think we might benefit from moving them to new location/class and see a additional functionality that could be used across. please do so.

options += uv_pip_compile_options_from_compiled_file(requirements_file)
command = "pyenv exec uv pip compile"
else
options += pip_compile_options_from_compiled_file(requirements_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

@avilaton , same as above

@jonjanego
Copy link
Member

Hi @beagold, @avilaton and other contributors. How is progress going on these changes?

@beagold
Copy link

beagold commented Sep 23, 2024

Hey!

I am waiting for a review on the documentation pr I made, and I believe @avilaton has some changes pending on this pull request (as requested by a dev here).

I could take a look at what is needed here and propose a patch. Will have a look later today!

@jonjanego
Copy link
Member

Hey!

I am waiting for a review on the documentation pr I made, and I believe @avilaton has some changes pending on this pull request (as requested by a dev here).

I could take a look at what is needed here and propose a patch. Will have a look later today!

I know there are some internal gates on the docs side, but I can help us work through those. Want to make sure that the code changes are moving forward though :)

@avilaton
Copy link
Author

Hi @beagold, @avilaton and other contributors. How is progress going on these changes?

I don't want to delay this a lot, I meant it to be a surgical change on the command and it snowballed into supporting other options which I don't fully understand. I need help here. The current pip-compile way of doing this has options parsed in both the file_updater and the version_resolver side of things, which are not quite the same.
I don't want to alter ANY code path taken by current pip-compile use-cases, this far exeeds what I was going for here.

@beagold
Copy link

beagold commented Sep 24, 2024

@avilaton hi!

I think the tests are failing due to your recent changes (I believe they were already failing before tho).

Im sorry to hear that this got a bit out for control for you. Ruby is still a bit new to me, but will try to spend the day today figuring out how to run and test dependabot locally and what changes still need to be done.

Again, thanks for getting this rolling!

def uv_pip_compile_options_from_compiled_file(requirements_file)
options = ["--output-file=#{requirements_file.name}"]

options << "--no-emit-index-url" unless requirements_file.content.include?("index-url http")
Copy link

@RazerM RazerM Sep 26, 2024

Choose a reason for hiding this comment

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

pip-tools defaults to --emit-index-url, but will skip the default index.

uv defaults to --no-emit-index-url so I think this should be

Suggested change
options << "--no-emit-index-url" unless requirements_file.content.include?("index-url http")
options << "--emit-index-url" if requirements_file.content.include?("index-url http")

Comment on lines +510 to +512
if (resolver = RESOLVER_REGEX.match(requirements_file.content))
options << "--resolver=#{resolver}"
end
Copy link

Choose a reason for hiding this comment

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

I don't think there's any reason to pass this to uv, since it's just there for better UX

@imajes
Copy link

imajes commented Sep 27, 2024

@avilaton, @beagold, @jonjanego, @sachin-sandhu, @RazerM -- to help out the team here, i applied the comments/fixed bugs in PR #10688. Hope that helps land this faster! Happy weekend :)

@imajes
Copy link

imajes commented Sep 30, 2024

OK, quick update- this isn't going to work as is. There are other bugs in it also, and now taking a deeper look at the way pdm/uv/pip-compile/etc are supported (or not) here, that's showing up a bit more to do. will come back to it shortly. @jonjanego - can you reach out to me please? thanks!

@jonjanego
Copy link
Member

Hi all; any further progress on this PR?

@avilaton
Copy link
Author

avilaton commented Oct 15, 2024 via email

@jerbob92
Copy link
Contributor

I would like to take over this PR, @avilaton can you tell me which bugs are still in here?

@mistercrunch
Copy link

mistercrunch commented Oct 30, 2024

Hey, wondering if there could be a way to tell dependabot which command to run, maybe in a .dependabot file (or similar) in place of the normal uv pip compile. For example people might want to specify arguments to uv pip compile, or in our case we want to run multiple commands atomically (one for requirements/base.in and another for requirements/dev.in) for instance. More details here: astral-sh/uv#5487 (comment)

An alternative would be to implement semantics on the uv side, meaning we'd tell uv that uv pip compile should run a certain set of commands.

More generally, at the dependabot level, it'd be great to allow people to alter the behavior/command(s) execute by dependabot to support their workflow, whether it's passing specific arguments or running multiple commands.

@beagold
Copy link

beagold commented Oct 30, 2024

Hey, wondering if there could be a way to tell dependabot which command to run, maybe in a .dependabot file (or similar) in place of the normal uv pip compile. For example people might want to specify arguments to uv pip compile, or in our case we want to run multiple commands atomically (one for requirements/base.in and another for requirements/dev.in) for instance. More details here: astral-sh/uv#5487 (comment)

An alternative would be to implement semantics on the uv side, meaning we'd tell uv that uv pip compile should run a certain set of commands.

More generally, at the dependabot level, it'd be great to allow people to alter the behavior/command(s) execute by dependabot to support their workflow, whether it's passing specific arguments or running multiple commands.

Probably not the place to ask for this, but dependabot will use the same flags that were used then the file was initially generated, so it has implicit support for this

@mistercrunch
Copy link

mistercrunch commented Oct 31, 2024

Probably not the place to ask for this

I agree but now that we're derailed I'll keep pushing :)

So dependabot will read the comment at the top of requirements.txt file to figure out what ran it initially and run the same command? Will it do it if there are many requirement output files, if so on the same PR or different PRs?

I remember digging in the docs to try and figure out how to pass specific arguments to pip-compile or specify multiple files and figured it wasn't possible. Good to know, not sure where it might be or should be documented as I think most software engineers wouldn't intuitively think that the bot is parsing the comments in a file to come up with a plan of what commands to execute.

Side note: it explains some of the magic behavior I've seen in some repos and found myself wondering what incantations were running behind the scene.

@ddelange
Copy link

fwiw, we get a backwards compatible output using --no-annotate --no-header:

uv pip compile requirements/*.txt -o requirements/requirements.lock --no-annotate --no-header

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.