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

allow failed import of Pool from multiprocessing #8162

Closed
wants to merge 2 commits into from
Closed

allow failed import of Pool from multiprocessing #8162

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17f622f
allow failed import of Pool from multiprocessing
landfillbaby Apr 28, 2020
d97b342
move comment
landfillbaby Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/8161.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix ``list --outdated`` and ``list --uptodate` on platforms without ``sem_open``
22 changes: 14 additions & 8 deletions src/pip/_internal/commands/list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,14 +213,20 @@ def latest_info(dist):
# This is done for 2x speed up of requests to pypi.org
# so that "real time" of this function
# is almost equal to "user time"
Copy link
Member

Choose a reason for hiding this comment

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

This comment should likely be rewritten.

Copy link
Member

Choose a reason for hiding this comment

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

... or just moved inside the "try" block.

pool = Pool(DEFAULT_POOLSIZE)

for dist in pool.imap_unordered(latest_info, packages):
if dist is not None:
yield dist

pool.close()
pool.join()
try:
pool = Pool(DEFAULT_POOLSIZE)

for dist in pool.imap_unordered(latest_info, packages):
if dist is not None:
yield dist

pool.close()
pool.join()
except ImportError:
Copy link
Contributor

@CrafterKolyan CrafterKolyan Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am not an expert of when Python actually imports class Pool and raises ImportError but in my opinion it should be in the actual importing code:

from multiprocessing.dummy import Pool

In CPython they wrap importing of module in try except clause.
(https://github.com/python/cpython/blob/d9a43e20facdf4ad10186f820601c6580e1baa80/Lib/multiprocessing/synchronize.py#L24-L33)

So the except part will never be executed in current implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it might be cleaner to move both for loops in a fetch_latest_info_multithreaded and fetch_latest_info_single_threaded, and then include the from multiprocessing.dummy import Pool in the fetch_latest_info_multithreaded function

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think the ImportError block needs to be put on the from multiprocessing.dummy line instead.

Copy link
Author

@landfillbaby landfillbaby Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yeah I was wondering if this reliance on delayed import would be allowed, I'll change it if it's really necessary
I'm not sure what you mean by "the except part will never be executed" though @CrafterKolyan, since that exception is exactly what caused this issue; raising from within an except block still raises.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not exactly sure from the original bug report which statement is triggering the ImportError. We should probably determine that, and just wrap that in the try...except (maybe setting a flag that we test later to choose the right code path).

Also, should this code have a test? Otherwise there's a (small) risk that the two implementations get out of line.

for dist in packages:
dist = latest_info(dist)
if dist is not None:
yield dist

def output_package_listing(self, packages, options):
packages = sorted(
Expand Down