GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-29842: Make Executor.map less eager so it handles large/unbounded… by MojoVampire · Pull Request #707 · python/cpython

Conversation

@MojoVampire

@MojoVampire MojoVampire commented Mar 18, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

… input iterables appropriately

https://bugs.python.org/issue29842

https://bugs.python.org/issue29842

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sadovnychyi, pheanex, chkno, graingert, hfudev, d-ryzhykau, illagrenan, Tronic, and b0ri5 reacted with thumbs up emoji
All reactions
  • 👍 9 reactions
@MojoVampire
bpo-29842: Make Executor.map less eager so it handles large/unbounded…
fbdb56c 
… input iterables appropriately
@the-knights-who-say-ni

the-knights-who-say-ni commented Mar 18, 2017

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
  4. If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
  5. Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Mar 18, 2017
@mention-bot

mention-bot commented Mar 18, 2017

Copy link
Copy Markdown

@MojoVampire, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @brianquinlan and @ezio-melotti to be potential reviewers.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MojoVampire

MojoVampire commented Mar 18, 2017

Copy link
Copy Markdown
Contributor Author

I've already submitted a contributor agreement pre-GitHub migration. I just updated my b.p.o. user profile (josh.r) to link to my GitHub account name. Is that sufficient, or do I need to submit a new contributor agreement based on my GitHub e-mail address?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mariatta Mariatta removed the CLA not signed label Mar 18, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 18, 2017
@MojoVampire

MojoVampire commented Mar 18, 2017

Copy link
Copy Markdown
Contributor Author

Hmm... Is the failure of continuous-integration/travis-ci/pr something real? Clicking Details just tells me it can't find a python/cpython repository at all...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Mar 19, 2017
pkch
pkch suggested changes May 15, 2017
View reviewed changes
Comment thread Doc/library/concurrent.futures.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/concurrent/futures/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/concurrent/futures/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/concurrent/futures/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/concurrent/futures/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
pkch
pkch reviewed May 17, 2017
View reviewed changes
Comment thread Lib/concurrent/futures/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pkch

pkch commented May 17, 2017

Copy link
Copy Markdown

You can also take a look at my implementation that I uploaded to https://github.com/pkch/executors. It does something more like what I described in the issue tracker, the main benefit being that it's not blocking.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
@brettcannon brettcannon added the awaiting core review label Feb 21, 2018
@bearpelican bearpelican mentioned this pull request Mar 29, 2018
Adding LazyThreadPoolExecutor class fastai/fastai#262
Merged
MojoVampire
MojoVampire commented Jul 25, 2018
View reviewed changes
Comment thread Doc/library/concurrent.futures.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@methane
methane requested a review from pitrou July 25, 2018 21:33
methane
methane requested changes Jul 25, 2018
View reviewed changes

@methane methane left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM at code level.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting core review label Jul 25, 2018
@bedevere-bot

bedevere-bot commented Jul 25, 2018

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jul 25, 2018
methane
methane requested changes Jul 25, 2018
View reviewed changes
Comment thread Lib/concurrent/futures/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/concurrent/futures/process.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@leezu

leezu commented Nov 13, 2018

Copy link
Copy Markdown

@MojoVampire could you share your plans about this PR? Do you plan to drive it forward?
pheanex reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MojoVampire added 2 commits May 6, 2019 12:18
@MojoVampire
Merge branch 'master' into fix-issue-29842
6281379
@MojoVampire
Add prefetch info to docstrings
8394e34
@MojoVampire

MojoVampire commented May 6, 2019

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

I did not add a Misc/NEWS entry since the file no longer exists (it's autogenerated from commit messages now, correct?).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels May 6, 2019
@bedevere-bot

bedevere-bot commented May 6, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

@methane: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tirkarthi

tirkarthi commented May 6, 2019

Copy link
Copy Markdown
Member

I did not add a Misc/NEWS entry since the file no longer exists (it's autogenerated from commit messages now, correct?).

NEWS entries can be generated using blurb or blurb-it

Please see : https://devguide.python.org/committing/?highlight=news#what-s-new-and-news-entries
MojoVampire reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MojoVampire
Add Misc/NEWS entry
634a3de
@MojoVampire

MojoVampire commented May 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

Actually made the Misc/NEWS entry properly. Sorry for confusion; I haven't made a PR since the news Misc/NEWS regime began and didn't know about the blurb tool. Thanks for the assist @tirkarthi
tirkarthi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented May 6, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

@methane: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pitrou
pitrou requested changes May 6, 2019
View reviewed changes

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Some comments below. A significant issue is that this changes the behaviour of shutdown(wait=True) to not wait for completion of all pending futures. I don't think that's an acceptable change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/concurrent/futures/_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/concurrent.futures.rst

By default, a reasonable number of tasks are
queued beyond the number of workers, an explicit *prefetch* count may be
provided to specify how many extra tasks should be queued.

@pitrou pitrou May 6, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Using "chunks" here would be more precise than "tasks".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MojoVampire MojoVampire May 6, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The documentation for chunksize uses the phrasing "this method chops iterables into a number of chunks which it submits to the pool as separate tasks", and since not all executors even use chunks (ThreadPoolExecutor ignores the argument), I figured I'd stick with "tasks". It does kind of leave out a term to describe a single work item; the docs uses chunks and tasks as synonyms, with no term for a single work item.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_concurrent_futures.py
self.assertCountEqual(finished, range(10))
# No guarantees on how many tasks dispatched,
# but at least one should have been dispatched
self.assertGreater(len(finished), 0)

@pitrou pitrou May 6, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this change breaks compatibility. The doc for shutdown says:

If wait is True then this method will not return until all the pending futures are done executing and the resources associated with the executor have been freed.

So all futures should have executed, instead of being cancelled.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MojoVampire MojoVampire May 6, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

At the time I wrote it, it didn't conflict with the documentation precisely; the original documentation said that map was "Equivalent to map(func, *iterables) except func is executed asynchronously and several calls to func may be made concurrently.", but doesn't guarantee that any actual futures exist (it's implemented in terms of submit and futures, but doesn't actually require such a design).

That said, it looks like you updated the documentation to add "the iterables are collected immediately rather than lazily;", which, if considered a guarantee, rather than a warning, would make this a breaking change even ignoring the "cancel vs. wait" issue.

Do you have any suggestions? If strict adherence to your newly (as of late 2017) documented behavior is needed, I suppose I could change the default behavior from "reasonable prefetch" to "exhaustive prefetch", so when prefetch isn't passed, every task is submitted, but it would be kind of annoying to lose the "good by default" behavior of limited prefetching.

The reason I cancelled rather than waiting on the result is that I was trying to follow the normal use pattern for map; since the results are yielded lazily, if the iterator goes away or is closed explicitly (or you explicitly shut down the executor), you're done; having the outstanding futures complete when you're not able to see the results means you're either:

  1. Expecting the tasks to complete without running out the Executor.map (which doesn't work with Py3's map at all, so the analogy to map should allow it; if you don't run it out, you have no guarantees anything was done)
  2. Not planning to use any further results (in which case running any submitted but unscheduled futures means doing work no one can see the results of)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou May 6, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Actually, I think there are two problems to discuss:

  1. What happens when shutdown(wait=True) is called. Currently it waits for all outstanding tasks. I don't think we can change that (the explicit wait flag exists for a reason).
  2. Whether map() can be silently switched to a lazy mode of operation. There's a (perhaps minor) problem with that. Currently, if one of iterables raises an error, map() propagates the exception. With your proposal, the exception may be raised later inside the result iterator.

I think 2) might easily be worked around by introducing a separate method (lazy_map?).

It seems it would be good to discuss those questions on the mailing-list.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MojoVampire MojoVampire May 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yeah, the problem with using the "lazy_map" name is that it feels like recreating the same annoying distinctions between map and imap from the Py2 era, and it would actually have Executor.map (which is supposed to match map, which lazily consumes the input(s)) be less similar to map than Executor.lazy_map.

If it's necessary to gain acceptance, I could change the default behavior to use prefetch=sys.maxsize - self._max_workers. It would match the pre-existing behavior for just about anything that conceivably worked before (modulo the tiny differences in memory usage of deque vs. list for storing the futures) since:

  1. All tasks would be submitted fully up front, so shutdown(wait=True) would in fact wait on them (and no further calls to submit would occur in the generator, so submitting wouldn't occur post-shutdown, which would raise a RuntimeError and cause the cancellation
  2. It wouldn't be lazy for anything by default (it would either work eagerly or crash, in the same manner it currently behaves)

If you passed a reasonable prefetch, you wouldn't have these behaviors (and we should document that interaction), but at least existing code would continue to work identically.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou May 7, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't have a strong opinion. I think discussing those alternatives on the ML, to gather more opinions and arguments, would be useful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting change review label May 6, 2019
@bedevere-bot

bedevere-bot commented May 6, 2019

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label May 6, 2019
MojoVampire added 2 commits May 6, 2019 15:18
@MojoVampire
Remove trailing whitespace in docs
0152831
@MojoVampire
Fix behavior to follow test_free_reference requirements (generator ho…
3c38fab 
…lds no reference to result at the moment it yields

Reduce line lengths to PEP8 limits
@flavianh

flavianh commented Jul 20, 2019

Copy link
Copy Markdown

@MojoVampire I think you need to comment your PR with I have made the requested changes; please review again for it to pass
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@csabella
csabella requested review from methane and pitrou January 14, 2020 12:21
@graingert graingert mentioned this pull request Feb 20, 2020
bpo-29842: Make Executor.map less eager so it handles large/unbounded… #18566
Closed
@github-actions

github-actions Bot commented Feb 19, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 19, 2022
@rhettinger
rhettinger requested a review from applio May 10, 2022 21:56
@MojoVampire MojoVampire mannequin mentioned this pull request Apr 10, 2022
Make Executor.map work with infinite/large inputs correctly #74028
Closed
@erlend-aasland

erlend-aasland commented Jun 29, 2022

Copy link
Copy Markdown
Contributor

@MojoVampire, are you going to follow up this PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Jun 29, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Jun 30, 2022
@MojoVampire

MojoVampire commented Jul 12, 2022

Copy link
Copy Markdown
Contributor Author

@erlend-aasland: I'd like to apply this, but I never got any idea of what would constitute an acceptable final result. Executor.map is, frankly, useless for many of its intended purposes right now; in an effort to improve performance on huge inputs, you end up prefetching the entire input and pre-scheduling all the tasks before you can process any of them.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@erlend-aasland

erlend-aasland commented Jul 14, 2022

Copy link
Copy Markdown
Contributor

cc. @kumaraditya303, is this something you'd be interested in reviewing?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303

kumaraditya303 commented Jul 15, 2022

Copy link
Copy Markdown
Contributor

is this something you'd be interested in reviewing?

I'll take a look soon, but it seems there are two PRs for the same thing and this one has conflicts so maybe we should continue on #18566
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303

kumaraditya303 commented Jul 18, 2022

Copy link
Copy Markdown
Contributor

Closing this in favor of #18566, thanks for the PR!
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303 kumaraditya303 closed this Jul 18, 2022
@ebonnal ebonnal mentioned this pull request Dec 4, 2024
gh-74028: concurrent.futures.Executor.map: introduce buffersize param for lazier behavior #125663
Merged
@AA-Turner AA-Turner removed the pending The issue will be closed if no feedback is provided label Apr 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou Awaiting requested review from pitrou
@methane methane Awaiting requested review from methane
@applio applio Awaiting requested review from applio
1 more reviewer
@pkch pkch pkch requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

17 participants

@MojoVampire @the-knights-who-say-ni @mention-bot @pkch @bedevere-bot @leezu @tirkarthi @flavianh @erlend-aasland @kumaraditya303 @methane @pitrou @brettcannon @serhiy-storchaka @Mariatta @AA-Turner @ezio-melotti
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.