Upgrade resolvelib to 0.4.0. #334
Conversation
mach_nix/data/providers.py
Outdated
| # TODO: Figure out how multiple incompatible build specifiers | ||
| # should be combined. | ||
| if any((req.build and req.build != reqs[0].build for req in reqs)): | ||
| return [] |
There was a problem hiding this comment.
I think build is conda only, and I'm not sure how it should be dealt with here.
There was a problem hiding this comment.
Yes, the build is relevant to conda only. That means, as soon as any build is specified, it automatically excludes all providers other than conda.
It should definitely be valid to have more then one build constraint. For example = 1.* and = 1.1.* are two different build constraints which will allow for the build to be 1.1.5 for example.
One way to handle it would be to iterate over the builds and call all_candidates_sorted for each build. Then take the intersection of the resulting sets. It might be more performant to refactor the lower level functions to support a list of builds, but on the other hand all_candidates_sorted is cached and therefore performance penalties might not be too bad.
mach_nix/data/providers.py
Outdated
| matching_candidates = [c for c in all if c.ver in matching_versions] | ||
| return matching_candidates | ||
|
|
||
| def all_candidates_sorted(self, name, extras=None, build=None) -> Iterable[Candidate]: | ||
| candidates = list(self.all_candidates(name, extras, build)) | ||
| candidates.sort(key=lambda c: (ver_sort_key(c.ver))) | ||
| candidates.sort(key=lambda c: (ver_sort_key(c.ver)), reverse=True) |
There was a problem hiding this comment.
The order that matches should be returned has been reversed in 0.4.0.
| def find_matches(self, req): | ||
| return self.provider.find_matches(req) | ||
| def find_matches(self, reqs): | ||
| return self.provider.find_matches(reqs) |
There was a problem hiding this comment.
find_matches now takes a list of requirements (which are all going to be of the same package).
mach_nix/data/providers.py
Outdated
| # TODO: Figure out how multiple incompatible build specifiers | ||
| # should be combined. | ||
| if any((req.build and req.build != reqs[0].build for req in reqs)): | ||
| return [] |
There was a problem hiding this comment.
Yes, the build is relevant to conda only. That means, as soon as any build is specified, it automatically excludes all providers other than conda.
It should definitely be valid to have more then one build constraint. For example = 1.* and = 1.1.* are two different build constraints which will allow for the build to be 1.1.5 for example.
One way to handle it would be to iterate over the builds and call all_candidates_sorted for each build. Then take the intersection of the resulting sets. It might be more performant to refactor the lower level functions to support a list of builds, but on the other hand all_candidates_sorted is cached and therefore performance penalties might not be too bad.
| matching_candidates = [c for c in all if c.ver in matching_versions] | ||
| return matching_candidates |
There was a problem hiding this comment.
| matching_candidates = [c for c in all if c.ver in matching_versions] | |
| return matching_candidates | |
| matching_versions = set(matching_versions) | |
| matching_candidates = [c for c in all if c.ver in matching_versions] | |
| return matching_candidates |
There was a problem hiding this comment.
I think if we don't make matching_versions a set here we will have quadratic runtime behavior.
There was a problem hiding this comment.
I wonder how often the cost of that quadratic behaviour is outweighed by the cost of constructing the set? That said, it would probably make more sense to push the requirement filtering logic down into all_candidates now instead.
There was a problem hiding this comment.
I wonder how often the cost of that quadratic behaviour is outweighed by the cost of constructing the set?
It might increase the average runtime, but since we don't have an upper bound of the number of versions, wouldn't it be more important to lower the worst case runtime? Debugging those scalability issues might be painful later.
I see that you have started refactoring the lower level functions which is nice. Let me know once this is ready for review.
|
I think this is ready for review. It is based on #340. |
resolvelib is on 0.8.1, but I'm only upgrading one step here, to make it easier to deal with breaking changes.