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

Make it possible (for a reporter) to determine which candidate a requirement originates from #46

Closed
pradyunsg opened this issue Apr 27, 2020 · 9 comments · Fixed by #50
Labels
enhancement New feature or request

Comments

@pradyunsg
Copy link
Contributor

pradyunsg commented Apr 27, 2020

This would be useful for reporters, to determine why a certain requirement is added.

I'm imagining a evaluating hook, that's called at the start of the loop on _attempt_to_pin_criterion, prior to pinning but unconditionally (so that it doesn't actually have to match the criterion).

As an example, I want to be able to visualize false-starts / not-compatible-version checks, before they're pinned and currently, there's no way for the reporter to get this information.

@pradyunsg pradyunsg added the enhancement New feature or request label Apr 27, 2020
@pradyunsg
Copy link
Contributor Author

pradyunsg commented Apr 27, 2020

an hard-coded assumption that pinning would be called after adding_requirement

Actually, this doesn't work for "root" requirements (workaround: have logic to look at whether starting was called), or for candidates that get evaluated but not pinned (useful for showing mismatches / failures).

@pradyunsg
Copy link
Contributor Author

pradyunsg commented Apr 27, 2020

Alternatively, we could add a evaluating hook, that's called at the start of the loop on _attempt_to_pin_criterion.

diff --git a/src/resolvelib/resolvers.py b/src/resolvelib/resolvers.py
index 157eefa..60cdb32 100644
--- a/src/resolvelib/resolvers.py
+++ b/src/resolvelib/resolvers.py
@@ -218,6 +218,7 @@ class Resolution(object):
     def _attempt_to_pin_criterion(self, name, criterion):
         causes = []
         for candidate in reversed(criterion.candidates):
+            self._r.evaluating(candidate)
             try:
                 criteria = self._get_criteria_to_update(candidate)
             except RequirementsConflicted as e:

That'd make the order-of-relevant-reporter-hook-calls:

calling resolve
  adding  ('A', <SpecifierSet('>=1.0')>)
starting
eval      ('A', <Version('3.0.0')>)
  adding  C==2.0.0
  adding  B==3.0.0
  pin     ('A', <Version('3.0.0')>)
eval      ('C', <Version('2.0.0')>)
  pin     ('C', <Version('2.0.0')>)
eval      ('B', <Version('3.0.0')>)
  adding  C==3.0.0
backtrack ('C', <Version('2.0.0')>)
backtrack ('A', <Version('3.0.0')>)
eval      ('A', <Version('2.0.0')>)
  adding  C==1.0.0
  adding  B==2.0.0
  pin     ('A', <Version('2.0.0')>)
eval      ('C', <Version('1.0.0')>)
  pin     ('C', <Version('1.0.0')>)
eval      ('B', <Version('2.0.0')>)
  adding  C==2.0.0
backtrack ('C', <Version('1.0.0')>)
backtrack ('A', <Version('2.0.0')>)
eval      ('A', <Version('1.0.0')>)
  adding  B==1.0.0
  pin     ('A', <Version('1.0.0')>)
eval      ('B', <Version('1.0.0')>)
  adding  C==1.0.0
  pin     ('B', <Version('1.0.0')>)
eval      ('C', <Version('1.0.0')>)
  pin     ('C', <Version('1.0.0')>)

To:

calling resolve
  adding  ('A', <SpecifierSet('>=1.0')>)
starting
  adding  C==2.0.0
  adding  B==3.0.0
  pin     ('A', <Version('3.0.0')>)
  pin     ('C', <Version('2.0.0')>)
  adding  C==3.0.0
backtrack ('C', <Version('2.0.0')>)
backtrack ('A', <Version('3.0.0')>)
  adding  C==1.0.0
  adding  B==2.0.0
  pin     ('A', <Version('2.0.0')>)
  pin     ('C', <Version('1.0.0')>)
  adding  C==2.0.0
backtrack ('C', <Version('1.0.0')>)
backtrack ('A', <Version('2.0.0')>)
  adding  B==1.0.0
  pin     ('A', <Version('1.0.0')>)
  adding  C==1.0.0
  pin     ('B', <Version('1.0.0')>)
  pin     ('C', <Version('1.0.0')>)

@pradyunsg pradyunsg changed the title Add information about candidate to BaseReporter.adding_requirement Make it possible (for a reporter) to determine which candidate a requirement originates from Apr 27, 2020
@uranusjr
Copy link
Member

I don’t like having too many hooks being called for a given code path. Would it be okay if a hook is added to report a candidate does not match? (i.e. for each candidate, either pinning or ignoring is called). Or maybe we can just renaming the pinning hook to something like tried_to_pin(candidate: Candidate, success: bool) and call it before the continue.

@techalchemy
Copy link
Member

Or maybe we can just renaming the pinning hook to something like tried_to_pin(candidate: Candidate, success: bool)

I for one prefer this option, fwiw, since the naming is a lot easier to understand - although I can see the advantage of having two completely separate calls as well to avoid having to embed the conditional logic in the reporter.

@pradyunsg
Copy link
Contributor Author

The other option is to add a parent argument to Reporter.adding_requirement -- the call site has the relevant information available to it.

@pradyunsg
Copy link
Contributor Author

Or maybe we can just renaming the pinning hook to something like tried_to_pin(candidate: Candidate, success: bool) and call it before the continue.

This doesn't make it possible to have all the information about which candidate the requirement originated from (see adding C==3.0.0 in the aforementioned output, which originates from ('B', <Version('2.0.0')>)).

@uranusjr
Copy link
Member

I can always pass in the requirements as well. How about

tried_to_pin(requirements: List[Requirement], candidate: Optional[Candidate])

where candidate would be None on a failed pin?

@pradyunsg
Copy link
Contributor Author

@uranusjr What would List[Requirement] be?

@uranusjr
Copy link
Member

The requirements that the resolver tried to find the intersection on. But now I see that’s not what you want, but parent from the criteria?

I think I’ll go with the parent in adding_requirement() solution. That is the most straightforward conceptually for me. It’s getting muddy what exactly are needed right now, so many options have been proposed. Would you mind creating a PR adding the needed hooks for this solution?

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 a pull request may close this issue.

3 participants