Fix reject of valid state in criteria compatibility check #111
Conversation
src/resolvelib/resolvers.py
Outdated
| for key, criterion in criteria.items(): | ||
| criteria[key] = Criterion( | ||
| criterion.candidates, | ||
| # TODO: is empty information allowed? |
There was a problem hiding this comment.
Empty information is technically allowed but likely unexpected, since it records where a criterion came from, and if a criterion comes from nowhere, should it not be removed?
There was a problem hiding this comment.
My reasoning was that we're dropping the constraint provided by a parent because we're dropping the pin on that parent. But if the new pin on the parent happens to have the same constraint (the common case), we might want to keep incompatibilities, in order to prevent backtracking over the same thing again.
But as I type this I realize that I'm really not yet confident in my understanding of how incompatibilities are gathered. So if you are confident that we can safely drop the criterion when no information remains I'll apply that change. Otherwise I'll have to sink my teeth in the incompatibilities some more.
There was a problem hiding this comment.
If the intent is to keep incompatibilities in the graph, I think it’s OK to keep a criterion with empty information (it’s basically an orphan node). We should describe this in the release note (by adding a release note fragment file in news) so provider/reporter implementers are aware of the possibility that information could be empty (and what that means), it’s likely no-one would be particular troubled by this since an orphan node can simply be ignored in the final graph anyway.
There was a problem hiding this comment.
All right, I'll do that. I think pip will need a minor change, but there seems to already be a breaking change in master compared to the last release anyway so I guess that is acceptable.
There was a problem hiding this comment.
I think the orphan nodes will be excluded automatically by build_result()
There was a problem hiding this comment.
Indeed they will for the final result, but not for intermediate calls to the provider, see here: #91 (comment).
There was a problem hiding this comment.
I've made an attempt at the news fragment but I'm not entirely sure about your change entry conventions.
Co-authored-by: Tzu-ping Chung <[email protected]>
Co-authored-by: Frost Ming <[email protected]>
|
Hi, I've accepted this PR for hacktoberfest, can you fix the lint issue by formatting the code with black? |
tests/test_resolvers.py
Outdated
| @@ -7,6 +21,12 @@ | |||
| ResolutionImpossible, | |||
| Resolver, | |||
| ) | |||
| from resolvelib.resolvers import Resolution # type: ignore | |||
There was a problem hiding this comment.
Resolution is not part of the public interface declared in src/resolvelib/resolvers.pyi. My reasoning was that for testing that does not matter. I meant to ask about this but I forgot about it. The error is as follows:
tests/test_resolvers.py:24: error: Module "resolvelib.resolvers" has no attribute "Resolution"; maybe "ResolutionError"?
Found 1 error in 1 file (checked 1 source file)
There was a problem hiding this comment.
We can add Resolution to the type stubs; I think someone would need it at some point anyway.
There was a problem hiding this comment.
All right, I'll do that, thanks.
There was a problem hiding this comment.
Done. I did have to add another more specific type ignore because the test case monkeypatches a private method. I think that should be acceptable?
| # This should be a NamedTuple, but Python 3.6 has a bug that prevents it. | ||
| # https://stackoverflow.com/a/50531189/1376863 |
There was a problem hiding this comment.
I copied this from another tuple at the top of the file.
Fix incorrect rejection of some states where dependencies for multiple versions of the same package are considered for conflicts. This pull request introduces a check at the end of each round of pinning: any newly invalidated pin (because of a new requirement), is removed as an information provider from the state's criteria.
I believe this implements the same behavior as initially proposed in #91 but more directed, to maintain the overall correctness of each state's criteria and to limit overhead.
I am currently still unsure how I should address this comment (last remaining TODO in the diff) but I thought I'd already open the bulk of my changes for review.
(If accepted, might I ask to add the hacktoberfest-accepted label to the pull request? I mean to follow through on this either way of course.)
closes #91