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

Annotate sources of requirements #3269

Merged
merged 36 commits into from
May 9, 2024

Conversation

palfrey
Copy link
Contributor

@palfrey palfrey commented Apr 25, 2024

Summary

Fixes #1343. This is kinda a first draft at the moment, but does at least mostly work locally (barring some bits of the test suite that seem to not work for me in general).

Test Plan

Mostly running the existing tests and checking the revised output is sane

Outstanding issues

Most of these come down to "AFAIK, the existing tools don't support these patterns, but uv does" and so I'm not sure there's an existing good answer here! Most of the answers so far are "whatever was easiest to build"

  • Is "-r pyproject.toml" correct? Should it show something else or get skipped entirely No it wasn't. Fixed in 3044fa8
  • If the requirements file is stdin, that just gets skipped. Should it be recorded?
  • Overrides get shown as "--override<override.txt>". Correct?
  • Some of the tests (e.g. dependency_excludes_non_contiguous_range_of_compatible_versions) make assumptions about the order of package versions being outputted, which this PR breaks. I'm not sure if the text is fairly arbitrary and can be replaced or whether the behaviour needs fixing? - fixed by removing the custom pubgrub PartialEq/Hash
  • Are all the TrackedFromStr et al changes needed, or is there an easier way? I don't think so, I think it's necessary to track these sort of things fairly comprehensively to make this feature work, and this sort of invasive change feels necessary, but happy to be proved wrong there :)
  • If you have a requirement coming in from two or more different requirements files only one turns up. I've got a closed-source example for this (can go into more detail if needed), mostly consisting of a complicated set of common deps creating a larger set. It's a rarer case, but worth considering. 042432b
  • Doesn't add annotations for setup.py yet
    • This is pretty hard, as the correct location to insert the path is crates/pypi-types/src/metadata.rs's parse_pkg_info, which as it's based off a source distribution has entirely thrown away such matters as "where did this package requirement get built from". Could add "built package name" as a dep, but that's a little odd.

use std::path::Path;

/// Like FromStr, but with information about where the str came from
pub trait TrackedFromStr: Sized {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not entirely sure this is the right package, but AFAIK, it's the only crate that everything that needs it can depend on....

Copy link
Member

Choose a reason for hiding this comment

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

I want to experiment with a slightly different approach here. Instead of a trait, could we just always use None for the source, and then add a method to set the source after parsing? Since the parsing itself doesn't depend on the source at all, right?

Copy link
Member

Choose a reason for hiding this comment

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

(I can give it a try.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. If you can make it work, all good!

@ThiefMaster
Copy link

ThiefMaster commented Apr 25, 2024

I just tested it with the requirements files of https://github.com/indico/indico/, and for requirements.{in,txt} it works great - no changes beyond the comments on top! :)

For requirements.dev.{in,txt}, however, I noticed one bug: Instead of -c requirements.txt it uses -r requirements.txt in the source comments. That aside, it seems to do everything fine there as well.

@zanieb
Copy link
Member

zanieb commented Apr 25, 2024

Exciting! I'm not a good review for this one, but I wonder if it could be used to address #1854 next?

@@ -60,6 +59,9 @@ pub enum PubGrubPackage {
/// version before the registry version. So we could just error if we visit a URL variant
/// _after_ a registry variant.
Option<VerbatimUrl>,
/// Names of the file sources of this package (e.g. requirements.in), or empty for dependencies
/// only from other libraries
Vec<SourceName>,
Copy link
Member

Choose a reason for hiding this comment

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

Could we instead omit this here, and re-connect the requirements to their paths when we build the ResolutionGraph?

Copy link
Member

Choose a reason for hiding this comment

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

I think that could also enable correct annotation for (e.g.) -c requirements.txt, since then you could identify requirements that came from constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started writing "here's why this doesn't work" and then found out in the course of writing it, that yes, it can be done! A tool I really want but can't seem to find with a bit of googling is some sort of code visualisation thing for Rust that'd let me visually see all of this v.s. the "lets cram it all into my head" approach I've been using, which clearly isn't the best option.

Here's my notes:

The only code that actually notices -r/-c requirements is parse_entry in the requirements-txt crate, which then only gets used by parse/parse_inner in RequirementsTxt. This results in path data in RequirementEntry and source values in the Requirement's that are the constraints in the RequirementsTxt struct.

That in turn gets called by RequirementsSpecification from_source/from_sources, which similarly builds requirements/constraints with path/source data.

pip_compile in the uv crate then calls all that, feeds it into the pubgrub bits for resolution, and then feeds the results into DisplayResolutionGraph for display.

So, yeah. I could take the data from RequirementsSpecification, fish out a package -> sources hashmap and skip adding it to Pubgrub entirely... let me have a look at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Also resolved some test failures :)

@palfrey
Copy link
Contributor Author

palfrey commented Apr 26, 2024

I just tested it with the requirements files of https://github.com/indico/indico/, and for requirements.{in,txt} it works great - no changes beyond the comments on top! :)

For requirements.dev.{in,txt}, however, I noticed one bug: Instead of -c requirements.txt it uses -r requirements.txt in the source comments. That aside, it seems to do everything fine there as well.

9655e91 should fix that.

@palfrey
Copy link
Contributor Author

palfrey commented Apr 26, 2024

@charliermarsh I'm seeing some test failures on Windows, and they appear to be related to path redactions in insta. There's some nice work for doing that cross-platform in crates/uv/tests/common/mod.rs, but I'm needing it for things like crates/requirements-txt/src/lib.rs and that would be a circular dependency. Any thoughts on how to approach sorting those out? I don't have an easy way to do Windows work sadly, so this might be a bit fiddly!

@ThiefMaster
Copy link

9655e91 should fix that.

Indeed, the output is exactly what I expect now!

[adrian@claptrap:~/dev/indico/src:master *$]> git diff
diff --git a/docs/requirements.txt b/docs/requirements.txt
index af095861bf..e16ac62be7 100644
--- a/docs/requirements.txt
+++ b/docs/requirements.txt
@@ -1,9 +1,5 @@
-#
-# This file is autogenerated by pip-compile with Python 3.12
-# by the following command:
-#
-#    pip-compile --strip-extras docs/requirements.in
-#
+# This file was autogenerated by uv via the following command:
+#    uv pip compile -o docs/requirements.txt docs/requirements.in
 alabaster==0.7.16
     # via sphinx
 babel==2.14.0
diff --git a/requirements.dev.txt b/requirements.dev.txt
index 6146f0535c..3fdd46a5bd 100644
--- a/requirements.dev.txt
+++ b/requirements.dev.txt
@@ -1,9 +1,5 @@
-#
-# This file is autogenerated by pip-compile with Python 3.12
-# by the following command:
-#
-#    pip-compile --strip-extras requirements.dev.in
-#
+# This file was autogenerated by uv via the following command:
+#    uv pip compile -o requirements.dev.txt requirements.dev.in
 aiosmtpd==1.4.5
     # via pytest-localserver
 alabaster==0.7.16
@@ -125,6 +121,8 @@ parso==0.8.4
     #   pyquotes
 pep8-naming==0.13.3
     # via -r requirements.dev.in
+pip==24.0
+    # via pip-tools
 pip-tools==7.4.1
     # via -r requirements.dev.in
 plantweb==1.2.1
@@ -190,6 +188,10 @@ ruff==0.4.1
     # via -r requirements.dev.in
 schemainspect==3.1.1663587362
     # via migra
+setuptools==69.5.1
+    # via
+    #   flake8-quotes
+    #   pip-tools
 six==1.16.0
     # via
     #   -c requirements.txt
@@ -267,7 +269,3 @@ werkzeug==3.0.2
     #   pytest-localserver
 wheel==0.43.0
     # via pip-tools
-
-# The following packages are considered to be unsafe in a requirements file:
-# pip
-# setuptools
diff --git a/requirements.txt b/requirements.txt
index 32521e6a04..a8ec374820 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,9 +1,5 @@
-#
-# This file is autogenerated by pip-compile with Python 3.12
-# by the following command:
-#
-#    pip-compile --strip-extras
-#
+# This file was autogenerated by uv via the following command:
+#    uv pip compile -o requirements.txt requirements.in
 alembic==1.13.1
     # via
     #   -r requirements.in

@jardarbo
Copy link

jardarbo commented Apr 29, 2024

@palfrey Very nice of you to look at this!

Testing your version with the following pyproject.toml:

$ cat pyproject.toml
[project]
name = "uv_test"
dynamic = ["version"]
dependencies = ["click"]

Running uv pip compile with your version yields the following:

$ uv pip compile pyproject.toml
Resolved 1 package in 4ms
# This file was autogenerated by uv via the following command:
#    uv pip compile pyproject.toml
click==8.1.7
    # via -r pyproject.toml

Running pip-compile (https://github.com/jazzband/pip-tools) yields:

$ pip-compile pyproject.toml
#
# This file is autogenerated by pip-compile with Python 3.12
# by the following command:
#
#    pip-compile pyproject.toml
#
click==8.1.7
    # via uv_test (pyproject.toml)

The difference is # via -r pyproject.toml vs. # via uv_test (pyproject.toml).

I don't know if you think we should match the output of pip-compile, but it makes it easier to mix the usage of uv pip compile and pip-compile.

Running this on Ubuntu on Windows 11 WSL.

@ThiefMaster
Copy link

+1 on matching the pip-compile output, it makes transitioning from pip-tools to uv much more straightforward (as you immediately see that nothing changed), and -r pyproject.toml seems weird anyway (unless pip install -r pyproject.toml is valid which I'd find surprising)

@palfrey
Copy link
Contributor Author

palfrey commented Apr 29, 2024

The difference is # via -r pyproject.toml vs. # via uv_test (pyproject.toml).

I don't know if you think we should match the output of pip-compile, but it makes it easier to mix the usage of uv pip compile and pip-compile.

I didn't investigate this path much, but had seen -r pyproject.toml and wasn't sure offhand how this was meant to be handled. Now I know, 3044fa8 fixes this.

@jardarbo
Copy link

Now I know, 3044fa8 fixes this.

Wow, this was quick @palfrey, thanks! This does indeed make output same as pip-compile.

$ uv pip compile pyproject.toml
Resolved 1 package in 4ms
# This file was autogenerated by uv via the following command:
#    uv pip compile pyproject.toml
click==8.1.7
    # via uv-test (pyproject.toml)

@charliermarsh
Copy link
Member

I have a Windows machine so I can debug on that.

@palfrey
Copy link
Contributor Author

palfrey commented May 8, 2024

BTW, it doesn't seem to add sources for editables e.g. the poetry_editable = { path = "../poetry_editable", editable = true } in tool_uv_sources just comes out as -e ../poetry_editable with no source. This might well be like the setup.py scenario I listed in the issues in the description, and may well be effectively unsolvable (or at least worth pushing further down the line post this PRs merge)

Nope :) Got a local fix, pushing shortly...

@palfrey
Copy link
Contributor Author

palfrey commented May 8, 2024

eb3ea98 fixes editables. I needed to change the sources key to String to cope with both package names and editables urls, which I'm a little uncertain about.

Just going to fix that clippy issue, then get some sleep 💤. Thanks for the review and work here!

@charliermarsh charliermarsh force-pushed the annotate-requirements-sources branch 2 times, most recently from f074664 to 21cd0fe Compare May 9, 2024 01:42
@charliermarsh charliermarsh force-pushed the annotate-requirements-sources branch from 21cd0fe to 0ebe593 Compare May 9, 2024 01:45
@charliermarsh charliermarsh force-pushed the annotate-requirements-sources branch from 718f35f to e09fe28 Compare May 9, 2024 02:08
@charliermarsh charliermarsh force-pushed the annotate-requirements-sources branch 2 times, most recently from 273c27e to f3181ce Compare May 9, 2024 03:07
@charliermarsh charliermarsh force-pushed the annotate-requirements-sources branch from f3181ce to 14630d2 Compare May 9, 2024 03:10
@charliermarsh charliermarsh merged commit bc963d1 into astral-sh:main May 9, 2024
43 checks passed
@palfrey palfrey deleted the annotate-requirements-sources branch May 9, 2024 07:15
@palfrey
Copy link
Contributor Author

palfrey commented May 9, 2024

🥳

@zanieb
Copy link
Member

zanieb commented May 9, 2024

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv pip compile does not annotate direct dependencies e.g. via -r requirements.in
5 participants