prefix-dev / rip

Solve and install Python packages quickly with rip (pip in Rust)
https://prefix.dev
BSD 3-Clause "New" or "Revised" License
655 stars 23 forks source link

Select pre-releases like pip does #141

Closed wolfv closed 9 months ago

wolfv commented 10 months ago

I tried something slightly different here: matching pre-releases but being able to pass in additional options to the contains(...) function.

I was planning to add a HashMap<PackageId, bool> that would contain wether a given package only has pre-releases, however, we don't have the pacakge names available. However, now that I am thinking of it, we could just add it as a second field to each version spec. Or maybe we should just add the prerelease yes/no option to the version spec itself. Maybe we don't even need the extra options ...

baszalmstra commented 10 months ago

This seems rather complex. Why not store whether prerelase are supported in the version instead of in the versionset. You know all the information when all versions are constructed.

wolfv commented 10 months ago

Hmm, maybe a good idea. We still need to store wether the versionset refers to any prerelease if I am not mistaken.

  1. When creating the versionset, we check if any element refers to a prerelease, and if yes, we store that as a bool (then we get rid of the iter().any() in the contains call
  2. When creating the version(s), we check if all of them are pre-releases, and store that as a bool in each version
  3. we could still have the global option as option that we pass to contains.

Does that make sense @tdejager @baszalmstra ? :)

baszalmstra commented 10 months ago

I think we just need one boolean in the version and one in the versionset. Both define wether prereleases are allowed.

your 1 and 2 should be able to inform initial values there I guess!

I think the options can stay in the provider and be read to compute the booleans. I dont think we need to complicate resolvo.

wolfv commented 10 months ago

Yep, you might be right :) I think what I just pushed captures your idea (I can revert back to stock-resolvo with this).

I did this pretty quickly – I think the code can be made more beautiful :)

baszalmstra commented 10 months ago

Nice! Code could use some improvement indeed but I think the idea is solid!

tdejager commented 10 months ago

Implementation looks solid!

Note that with this PR we are still deviating from the spec (https://packaging.python.org/en/latest/specifications/version-specifiers/#handling-of-pre-releases) if I'm reading it correctly namely.

https://packaging.python.org/en/latest/specifications/version-specifiers/#handling-of-pre-releases

... accept remotely available pre-releases for version specifiers where there is no final or post release that satisfies the version specifier

So instead matching pre-releases, only if the project has pre-releases, the spec said we should match if these are the only options that match this version range. However, as Bas and others noted, this is a bit of strange requirement that can potentially lead to unwanted behavior. Also, in issue #118 another issue followed that they want to investigate the spec and pip implementation anyways. https://github.com/pypa/pip/issues/12471#issuecomment-1887273309

So I think it's fine at this point to deviate from spec, however, it should be noted in the code that we are doing so I think

wolfv commented 9 months ago

Added some docs, but no test yet. :)

notatallshaw commented 9 months ago

FYI, I've been testing this branch against the new apache-airflow, including the new 2.8.1 as they have completely revamped their packaging infrastructure: https://airflow.apache.org/docs/apache-airflow/stable/release_notes.html#airflow-packaging-specification-follows-modern-python-packaging-standards-36537

The pre-releases look good, but I think this exposes some other issues with what packages rip visits when resolving, I will retest and file new issues once this lands (as I am not able to merge this branch with main locally myself).

tdejager commented 9 months ago

Let's merge when we have fixed the open comments :)