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

Revert the part Candidate.dist is being eagerly computed #9288

Conversation

uranusjr
Copy link
Member

One of the changes in #9264 was to make the dist property eagerly computed since I thought the lazy candidate sequence would be good enough. But it turns out that’s not the case, since in --upgrade mode, the iterator held in FoundCandidates is actually eagerly consumed:

def _insert_installed(installed, others):
# type: (Candidate, Iterator[Candidate]) -> Iterator[Candidate]
"""Iterator for ``FoundCandidates``.
This iterator is used when the resolver prefers to upgrade an
already-installed package. Candidates from index are returned in their
normal ordering, except replaced when the version is already installed.
Since candidates from index are already sorted by reverse version order,
`sorted()` here would keep the ordering mostly intact, only shuffling the
already-installed candidate into the correct position. We put the already-
installed candidate in front of those from the index, so it's put in front
after sorting due to Python sorting's stableness guarentee.
"""
candidates = sorted(
itertools.chain([installed], others),
key=operator.attrgetter("version"),
reverse=True,
)
return iter(candidates)

So this PR restores the previous lazily-calculation of the dist property. I’m honestly not sure why we need to eagerly consume the iterator, and will do more experiements to see whether that can be eliminated. But this is the shortest route to fix 20.3.2 and should be published first.

@pradyunsg
Copy link
Member

FAILED tests/functional/test_new_resolver.py::test_new_resolver_skip_inconsistent_metadata

FYI

@uranusjr
Copy link
Member Author

That’s why we had to resolve the candidate eagerly—otherwise we don’t know where we can catch this exception. Maybe we should do #9288 instead?

@pradyunsg
Copy link
Member

Maybe we should do #9288

This is #9288. ;)

@uranusjr uranusjr closed this Dec 15, 2020
@uranusjr uranusjr force-pushed the new-resolver-do-not-eagerly-consume-in-upgrade-mode branch from 60a5dbc to 2b0b426 Compare December 15, 2020 09:28
@uranusjr
Copy link
Member Author

#9289. Too many open tabs 😩

@uranusjr uranusjr deleted the new-resolver-do-not-eagerly-consume-in-upgrade-mode branch December 15, 2020 09:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants