Filter out incompatible dists #398

konstin · 2023-11-10T19:21:26Z

Filter out source dists and wheels whose requires-python from the simple api is incompatible with the current python version.

This change showed an important problem: When we use a fake python version for resolving, building source distributions breaks down because we can only build with versions we actually have.

This change became surprisingly big. The tests now require python 3.7 to be installed, but changing that would mean an even bigger change.

Fixes #388

charliermarsh · 2023-11-10T19:28:52Z

crates/puffin-resolver/src/finder.rs

+                // https://github.com/astral-sh/puffin/issues/388
+                if file
+                    .requires_python
+                    .clone()


I'm surprised this needs to clone -- is it necessary? Should VersionSpecifiers from implement From<&str>?

charliermarsh · 2023-11-10T19:29:23Z

crates/puffin-resolver/src/resolver.rs

-                            if let std::collections::btree_map::Entry::Vacant(entry) =
-                                version_map.entry(version)
+                            // Only add source dists compatible with the python version
+                            // https://github.com/astral-sh/puffin/issues/388


I think you can omit the issue link here, folks can always look this up in the Git history.

charliermarsh · 2023-11-10T19:31:18Z

crates/pypi-types/src/simple_json.rs

@@ -15,7 +17,7 @@ pub struct File {
    pub data_dist_info_metadata: Metadata,
    pub filename: String,
    pub hashes: Hashes,
-    pub requires_python: Option<String>,
+    pub requires_python: Option<LenientVersionSpecifiers>,


I think the type here should be VersionSpecifiers, right? As it is in Metadata21?

Like I think the stored type should be VersionSpecifiers even if we use LenientVersionSpecifiers for parsing...

charliermarsh · 2023-11-10T20:57:04Z

crates/puffin-cli/src/commands/pip_compile.rs

+    let interpreter_info = venv
+        .interpreter_info()
+        .clone()
+        .patch_markers(markers.clone().into_owned());


Can you say more about this? Don't we want to mirror the real markers in the resolver?

This is so we use the fake python version everywhere including when recursing for builds, it's effectively #407

Does this mean we'll end up trying to build source distributions that aren't actually supported? E.g., if we're using Python 3.7, but we use --python-version py312, when recursing, would we try to build Python 3.12-only source distributions?

zanieb · 2023-11-10T22:28:57Z

crates/puffin-resolver/src/resolver.rs

-                            let version = PubGrubVersion::from(filename.version.clone());
-                            if let std::collections::btree_map::Entry::Vacant(entry) =
-                                version_map.entry(version)
+                            // Only add source dists compatible with the python version


Is this the right place to filter this?

It seems like it'd be ideal of we could reject these versions with a message (in the style of pubgrub-rs/pubgrub#152) so that the reason the versions are rejected are tracked in the derivation tree.

Yeah that would be better for sure. Resolvo has this concept too: dependencies that are available but don’t match the current platform. (I don’t see how we can support this right now though, and we already filter out incompatible wheels…)

Okay — the xref here will be enough for me to revisit if I get this functionality added to PubGrub.

Awesome, thank you!

Something like this, if that works in the pubgrub/resolvo models?

type VersionMap = BTreeMap<PubGrubVersion, MaybeDistFile> enum MaybeDistFile { /// No compatible wheels and no source distribution (e.g. only an `.exe`) NoCompatiblePackage, /// No compatible wheels and the source distribution isn't compatible VersionMismatch(VersionSpecifiers), /// Available { /// The highest priority file, usually a wheel picked: DistFile, /// Other installation candidates, such as lower priority compatible wheels and the source distribution if a wheel exists /// /// We track them here to add their hashes to the lockfile (the user may e.g. have a different glibc version) /// TODO(konstin): Check that this is actually how pip-tools generates the hashes list others: Vec<DistFile> } }

otoh it might make more sense to keep those sdists in and instead give them a dependency on the python version, so we can properly report a conflict on root requiring python ==3.7, while pandas at the selected version requires python >3.9

konstin · 2023-11-13T11:35:25Z

My suggestion would be to merge this because it unblocks a bunch of downstream things where we currently fail on picking an invalid sdist and add the proper #406 solution later

Filter out source dists whose `requires-python` from the simple api is incompatible with the current python version. This change showed an important problem: When we use a fake python version for resolving, building source distributions breaks down because we can only build with versions we actually have. This change became surprisingly big.

konstin · 2023-11-13T13:05:22Z

crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_markers.snap

 ---
 success: true
 exit_code: 0
 ----- stdout -----
 # This file was autogenerated by Puffin v0.0.1 via the following command:
 #    [BIN_PATH] pip-compile requirements.in --constraint constraints.txt --cache-dir [CACHE_DIR]
 anyio==4.0.0
+exceptiongroup==1.1.3


pip installs this one, too, so i assume it's correct

konstin · 2023-11-13T13:05:27Z

crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_txt.snap

@@ -22,7 +22,9 @@ asgiref==3.7.2
 django==5.0b1
 sqlparse==0.4.3
    # via django
+typing-extensions==4.8.0


pip installs this one, too, so i assume it's correct

konstin force-pushed the filter-out-sdists branch from e86cda4 to dbca662 Compare November 10, 2023 19:22

charliermarsh reviewed Nov 10, 2023

View reviewed changes

charliermarsh force-pushed the filter-out-sdists branch from 3a019c2 to cd2aed3 Compare November 10, 2023 20:53

charliermarsh reviewed Nov 10, 2023

View reviewed changes

zanieb reviewed Nov 10, 2023

View reviewed changes

konstin mentioned this pull request Nov 13, 2023

Treat python version as a dependency when resolving #406

Closed

konstin force-pushed the filter-out-sdists branch from cd2aed3 to 39d9a8b Compare November 13, 2023 09:15

konstin force-pushed the filter-out-sdists branch from 92d3a6d to d78a1a8 Compare November 13, 2023 11:50

konstin changed the title ~~Filter out incompatible source dists~~ Filter out incompatible dists Nov 13, 2023

And the wheels, too

fac7362

konstin commented Nov 13, 2023

View reviewed changes

charliermarsh mentioned this pull request Nov 13, 2023

Injecting a python version when building source distributions #407

Closed

charliermarsh approved these changes Nov 13, 2023

View reviewed changes

konstin added 2 commits November 13, 2023 17:02

Try fixing CI python version

2922d31

Run tests on actual python 3.12

881aeac

konstin merged commit 76a4106 into main Nov 13, 2023
3 checks passed

konstin deleted the filter-out-sdists branch November 13, 2023 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter out incompatible dists #398

Filter out incompatible dists #398

konstin commented Nov 10, 2023 •

edited

Loading

charliermarsh Nov 10, 2023

charliermarsh Nov 10, 2023

charliermarsh Nov 10, 2023 •

edited

Loading

charliermarsh Nov 10, 2023

charliermarsh Nov 10, 2023

konstin Nov 13, 2023

charliermarsh Nov 13, 2023

zanieb Nov 10, 2023

charliermarsh Nov 10, 2023 •

edited

Loading

zanieb Nov 10, 2023

charliermarsh Nov 10, 2023

konstin Nov 11, 2023

konstin Nov 13, 2023 •

edited

Loading

konstin commented Nov 13, 2023

konstin Nov 13, 2023

konstin Nov 13, 2023

Filter out incompatible dists #398

Filter out incompatible dists #398

Conversation

konstin commented Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstin Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

konstin commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstin commented Nov 10, 2023 •

edited

Loading

charliermarsh Nov 10, 2023 •

edited

Loading

charliermarsh Nov 10, 2023 •

edited

Loading

konstin Nov 13, 2023 •

edited

Loading