◐ Shell
reader mode source ↗
Skip to content

bpo-29842: Make Executor.map less eager so it handles large/unbounded…#707

Closed
MojoVampire wants to merge 6 commits into
python:mainfrom
MojoVampire:fix-issue-29842
Closed

bpo-29842: Make Executor.map less eager so it handles large/unbounded…#707
MojoVampire wants to merge 6 commits into
python:mainfrom
MojoVampire:fix-issue-29842

Conversation

@MojoVampire

@MojoVampire MojoVampire commented Mar 18, 2017

Copy link
Copy Markdown
Contributor

@the-knights-who-say-ni

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!

@mention-bot

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.

@MojoVampire

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?

@MojoVampire

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...

@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.

@methane methane requested a review from pitrou July 25, 2018 21:33

@methane methane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM at code level.

@bedevere-bot

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.

@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?

@MojoVampire

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?).

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

24 hidden items Load more…
@tirkarthi

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

MojoVampire commented May 6, 2019

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

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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.

@bedevere-bot

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.

…lds no reference to result at the moment it yields

Reduce line lengths to PEP8 limits
@flavianh

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

@github-actions

Copy link
Copy Markdown

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

@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
@erlend-aasland

Copy link
Copy Markdown
Contributor

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

@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 label Jun 30, 2022
@MojoVampire

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.

@ezio-melotti ezio-melotti removed the label Jul 13, 2022
@erlend-aasland

Copy link
Copy Markdown
Contributor

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

@kumaraditya303

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

@kumaraditya303

Copy link
Copy Markdown
Contributor

Closing this in favor of #18566, thanks for the PR!

@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

Labels

awaiting changes type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.