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.

Sign up for GitHub

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

Jump to bottom

Avoid preparing metadata when pip download with --ignore-requires-python --no-deps #9311

Closed
wants to merge 1 commit into from
Closed

Avoid preparing metadata when pip download with --ignore-requires-python --no-deps #9311

wants to merge 1 commit into from

Conversation

nchepanov
Copy link
Contributor

Depends on: #9305
Originated in: #1884

The goal of this PR is to avoid calling Candidate._prepare() when it's not necessary.

Without this change, the following call chain happens:

Candidate.iter_dependencies()
-> Candidate._get_requires_python_dependency()
-> Candidate._prepare()   <<<<< heavy unnecessary work
-> Factory.make_requires_python_requirement()
-> if self._ignore_requires_python and return None

  File "/dev/pip/src/pip/_internal/commands/download.py", line 130, in run
    requirement_set = resolver.resolve(
  File "/dev/pip/src/pip/_internal/resolution/resolvelib/resolver.py", line 121, in resolve
    self._result = resolver.resolve(
  File "/dev/pip/src/pip/_vendor/resolvelib/resolvers.py", line 445, in resolve
    state = resolution.resolve(requirements, max_rounds=max_rounds)
  File "/dev/pip/src/pip/_vendor/resolvelib/resolvers.py", line 339, in resolve
    failure_causes = self._attempt_to_pin_criterion(name, criterion)
  File "/dev/pip/src/pip/_vendor/resolvelib/resolvers.py", line 207, in _attempt_to_pin_criterion
    criteria = self._get_criteria_to_update(candidate)
  File "/dev/pip/src/pip/_vendor/resolvelib/resolvers.py", line 198, in _get_criteria_to_update
    for r in self._p.get_dependencies(candidate):
  File "/dev/pip/src/pip/_internal/resolution/resolvelib/provider.py", line 170, in get_dependencies
    return [
  File "/dev/pip/src/pip/_internal/resolution/resolvelib/provider.py", line 170, in <listcomp>
    return [
  File "/dev/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 264, in iter_dependencies
    yield self._get_requires_python_dependency()
  File "/dev/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 245, in _get_requires_python_dependency
    requires_python = get_requires_python(self.dist)
  File "/dev/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 240, in dist
    self._prepare()
  File "/dev/pip/src/pip/_internal/resolution/resolvelib/candidates.py", line 225, in _prepare

In other words, if this check was done earlier - we can avoid running candidate setup.py when --ignore-requires-python is given.

Another argument might be that this check doesn't belong to the Factory, and similar to iter_dependencies(with_requires) should be passed down to Candidate from Provider.get_dependencies().

@nchepanov nchepanov changed the title Pip download optimize Avoid preparing metadata when pip download with --ignore-requires-python --no-deps Dec 17, 2020
@nchepanov nchepanov closed this Dec 18, 2020
@nchepanov
Copy link
Contributor Author

nchepanov commented Dec 18, 2020 •
edited
Loading

This is the second time I'm seeing this failure happen on MacOS / Python 3.8 CI -- unrelated to this change.


INTERNALERROR>         pytest_internalerror() was called on the worker.
INTERNALERROR>     
INTERNALERROR>         pytest_internalerror() arguments are an excinfo and an excrepr, which can't
INTERNALERROR>         be serialized, so we go with a poor man's solution of raising an exception
INTERNALERROR>         here ourselves using the formatted message.
INTERNALERROR>         """
INTERNALERROR>         self._active_nodes.remove(node)
INTERNALERROR>         try:
INTERNALERROR> >           assert False, formatted_error
INTERNALERROR> E           AssertionError: Traceback (most recent call last):
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/_pytest/main.py", line 269, in wrap_session
INTERNALERROR> E                 session.exitstatus = doit(config, session) or 0
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/_pytest/main.py", line 323, in _main
INTERNALERROR> E                 config.hook.pytest_runtestloop(session=session)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/hooks.py", line 286, in __call__
INTERNALERROR> E                 return self._hookexec(self, self.get_hookimpls(), kwargs)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/manager.py", line 93, in _hookexec
INTERNALERROR> E                 return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/manager.py", line 84, in <lambda>
INTERNALERROR> E                 self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 208, in _multicall
INTERNALERROR> E                 return outcome.get_result()
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 80, in get_result
INTERNALERROR> E                 raise ex[1].with_traceback(ex[2])
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 187, in _multicall
INTERNALERROR> E                 res = hook_impl.function(*args)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/xdist/remote.py", line 72, in pytest_runtestloop
INTERNALERROR> E                 self.run_one_test(torun)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/xdist/remote.py", line 89, in run_one_test
INTERNALERROR> E                 self.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/hooks.py", line 286, in __call__
INTERNALERROR> E                 return self._hookexec(self, self.get_hookimpls(), kwargs)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/manager.py", line 93, in _hookexec
INTERNALERROR> E                 return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/manager.py", line 84, in <lambda>
INTERNALERROR> E                 self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 208, in _multicall
INTERNALERROR> E                 return outcome.get_result()
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 80, in get_result
INTERNALERROR> E                 raise ex[1].with_traceback(ex[2])
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 182, in _multicall
INTERNALERROR> E                 next(gen)  # first yield
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pytest_timeout.py", line 104, in pytest_runtest_protocol
INTERNALERROR> E                 timeout_setup(item)
INTERNALERROR> E               File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pytest_timeout.py", line 207, in timeout_setup
INTERNALERROR> E                 signal.signal(signal.SIGALRM, handler)
INTERNALERROR> E               File "/Users/runner/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/signal.py", line 47, in signal
INTERNALERROR> E                 handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
INTERNALERROR> E             ValueError: signal only works in main thread
INTERNALERROR> E           assert False
INTERNALERROR> 
INTERNALERROR> .tox/py/lib/python3.8/site-packages/xdist/dsession.py:187: AssertionError
INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/_pytest/main.py", line 269, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/_pytest/main.py", line 323, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/hooks.py", line 286, in __call__
INTERNALERROR>     return self._hookexec(self, self.get_hookimpls(), kwargs)
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/manager.py", line 93, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/manager.py", line 84, in <lambda>
INTERNALERROR>     self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 208, in _multicall
INTERNALERROR>     return outcome.get_result()
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 80, in get_result
INTERNALERROR>     raise ex[1].with_traceback(ex[2])
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/pluggy/callers.py", line 187, in _multicall
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/xdist/dsession.py", line 112, in pytest_runtestloop
INTERNALERROR>     self.loop_once()
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/xdist/dsession.py", line 135, in loop_once
INTERNALERROR>     call(**kwargs)
INTERNALERROR>   File "/Users/runner/work/pip/pip/.tox/py/lib/python3.8/site-packages/xdist/dsession.py", line 174, in worker_workerfinished
INTERNALERROR>     assert not crashitem, (crashitem, node)
INTERNALERROR> AssertionError: ('tests/functional/test_broken_stdout.py::test_broken_stdout_pipe__verbose', <WorkerController gw2>)
INTERNALERROR> assert not 'tests/functional/test_broken_stdout.py::test_broken_stdout_pipe__verbose'

============================ 512 warnings in 10.63s ============================
ERROR: InvocationError for command /Users/runner/work/pip/pip/.tox/py/bin/pytest --timeout 300 -m integration --verbose --numprocesses auto --durations=5 (exited with code 3)
___________________________________ summary ____________________________________
ERROR:   py: commands failed

@nchepanov nchepanov reopened this Dec 18, 2020
@pradyunsg
Copy link
Member

Yea, that's a pytest-xdist bug. :(

@uranusjr
Copy link
Member

The main issue I have with the current approach is that the call path of _prepare() is too obscure to track. This change avoids preparation, but it is too easy for someone to change some subtle thing and break the laziness.

In particular, #9289 moves the preparation into __init__(). This change was in 20.3.2, and reverted in 20.3 due to an implementation error. But this change will very likely happen eventually, since it is the best way to raise build-time errors in a predictable place, so they can be caused to trigger backtracking. Logic in this PR will break with that change.

So I think the best approach here instead is to analyse why _prepare() is called, and pass an explicit flag to avoid doing it when we don’t want to call it. This may require several iteration to get right (there are too many combinations, but should be what we go for.

pradyunsg
pradyunsg reviewed Dec 21, 2020
View reviewed changes
src/pip/_internal/resolution/resolvelib/base.py
@@ -143,8 +143,8 @@ def source_link(self):
# type: () -> Optional[Link]
raise NotImplementedError("Override in subclass")

def iter_dependencies(self, with_requires):
# type: (bool) -> Iterable[Optional[Requirement]]
def iter_dependencies(self, with_requires, ignore_requires_python):
Copy link
Member

@pradyunsg pradyunsg Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def iter_dependencies(self, with_requires, ignore_requires_python):
def iter_dependencies(self, with_requires, with_requires_python):

For consistency internally in the pip._internal.resolution.resolvelib package?

@nchepanov
Copy link
Contributor Author

The main issue I have with the current approach is that the call path of _prepare() is too obscure to track.

I'd like to suggest that this PR is still relevant by itself, because it decouples making a Requirement object via Factory.make_requires_python_requirement from the decision whether such requirements need to be made in the first place.

The patch moves the logic to where it belongs along with _ignore_dependencies check. Assuming I will accept @pradyunsg suggestion above, would you be willing to merge this? I can update the description to remove current rational and change it to something like "Decouple ...."

This change avoids preparation, but it is too easy for someone to change some subtle thing and break the laziness.

instead ... to analyze why _prepare() is called, and pass an explicit flag

I agree, in fact #1884 (comment) and this "work in progress" commit https://github.com/nikitos3000/pip/commit/391467d6be2363b651dba6f8fc2f94088f8e2937 are part of my attempt to decouple the download process from preparing metadata.

What do you think about this approach? If _prepare was moved to __init__() then the Factory would have to pass both ignore_requires_python and ignore_dependencies flags into the initializer call. Is this how you see it? Or maybe it can be a derived flag, e.g. Candidate(must_prepare=True)?

@uranusjr
Copy link
Member

I don’t think it’s a good idea to merge this without the benefit of skipping metadata preparation, since this PR would only move some arguments without changing anything. You may think it is better to have ignores_requires_python passed in as arguments (I don’t have strong objections), but that’s a subject thing, and difficult to justify if it does not really practically change anything. I believe there is a simpler solution to this based on #9289 if we step back from the immediate problem and try to analyse what makes metadata preparation unnecessary (instead of deciding it’s unnecessary and try to find a way to avoid that), and model the solution from that observation.

@nchepanov
Copy link
Contributor Author

I don’t think it’s a good idea to merge this without the benefit of skipping metadata preparation, since this PR would only move some arguments without changing anything.

agree, closing for now

@nchepanov nchepanov closed this Feb 12, 2021
@uranusjr
Copy link
Member

Now that #9289 is merged, I should put this back to my backlog and investigate what needs to change to make it happen.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nchepanov @pradyunsg @uranusjr