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

loop.run_in_executor should propagate current contextvars #78195

Closed
hellysmile mannequin opened this issue Jun 30, 2018 · 14 comments
Closed

loop.run_in_executor should propagate current contextvars #78195

hellysmile mannequin opened this issue Jun 30, 2018 · 14 comments
Labels
3.9 only security fixes topic-asyncio type-feature A feature request or enhancement

Comments

@hellysmile
Copy link
Mannequin

hellysmile mannequin commented Jun 30, 2018

BPO 34014
Nosy @asvetlov, @1st1, @hellysmile
PRs
  • bpo-34014: Added support of contextvars for BaseEventLoop.run_in_executor #8035
  • bpo-34014: Added support of contextvars for BaseEventLoop.run_in_executor #9688
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2022-03-23.20:57:22.475>
    created_at = <Date 2018-06-30.23:26:24.700>
    labels = ['type-feature', '3.9', 'expert-asyncio']
    title = 'loop.run_in_executor should propagate current contextvars'
    updated_at = <Date 2022-03-23.20:57:22.474>
    user = 'https://github.com/hellysmile'

    bugs.python.org fields:

    activity = <Date 2022-03-23.20:57:22.474>
    actor = 'asvetlov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-03-23.20:57:22.475>
    closer = 'asvetlov'
    components = ['asyncio']
    creation = <Date 2018-06-30.23:26:24.700>
    creator = 'hellysmile'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34014
    keywords = ['patch']
    message_count = 14.0
    messages = ['320814', '320817', '320818', '320819', '320820', '326812', '326813', '326950', '326972', '326975', '326978', '326981', '350467', '415908']
    nosy_count = 3.0
    nosy_names = ['asvetlov', 'yselivanov', 'hellysmile']
    pr_nums = ['8035', '9688']
    priority = 'normal'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34014'
    versions = ['Python 3.9']

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Jun 30, 2018

    PEP-567 supports copying context into another threads, but for asyncio users each call run_in_executor requires explicit context propagation

    For end users expected behavior that context is copied automatically

    @hellysmile hellysmile mannequin added 3.7 (EOL) end of life 3.8 only security fixes topic-asyncio labels Jun 30, 2018
    @1st1
    Copy link
    Member

    1st1 commented Jun 30, 2018

    I considered enabling that, but in the end decided not to. The reason is that it's possible to use a ProcessPoolExecutor with run_in_execuror(), and context cars currently don't support pickling (and probably never will). We can't have a single api that sometimes works with contextvars and sometimes doesn't.

    So this is a "won't fix".

    @1st1
    Copy link
    Member

    1st1 commented Jun 30, 2018

    As a workaround you can subclass the ThreadPoolExecutor and set it as a default executor.

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Jul 1, 2018

    What about to check instance of executor and depending on that propagate contextvars or not?

    As well to raise RuntimeError for ProcessPoolExecutor with provided context?

    I assume, that ProcessPoolExecutor as executor is not so popular in real world, but I can not provide any stats.

    Different behavior is defiantly at least weird, but do not support natively what is technically expected also strange.

    https://docs.python.org/3.7/library/contextvars.html#asyncio-support Context variables are natively supported in asyncio is hard for end users.

    Each use case of contextvars + run_in_executor will produce copy-paste boilerplate

    As for me it is very similar to the asyncio.get_event_loop() history

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Jul 1, 2018

    ProcessPoolExecutor as executor is not so popular in real world - as executor for asyncio

    @1st1
    Copy link
    Member

    1st1 commented Oct 1, 2018

    So we're deprecating passing non-ThreadPoolExecutor instances to loop.set_default_executor. In 3.9 that will trigger an error.

    For this issue we have basically the next few options:

    (1) Do nothing;

    (2) Fix "run_in_executor" to start copying and running in correct context automatically

    (3) Add a new keyword-only parameter to "run_in_executor": "retain_context=False"

    (4) Design a new async/await friendly API for using thread- and process-pools.

    Now, (4) will happen. The "run_in_executor" method is low-level and requires accessing the event loop to use it. We probably have enough time to design this new API before 3.8.

    As for implementing (3) in 3.8 -- I'd be OK with that too. Andrew, your thoughts?

    @1st1
    Copy link
    Member

    1st1 commented Oct 1, 2018

    One problem with (3) is what will happen if someone uses "retain_context=True" and a ProcessPoolExecutor. It has to fail in a graceful and obvious way.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 3, 2018

    I vote on not changing run_in_executor behavior if we cannot make it work with ProcessPoolExecutor.

    If a new API will solve the problem -- that's fine.
    Until it landed the explicit context propagation is the satisfactory solution.

    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Oct 3, 2018

    I've created new pull request #9688 with the implementation of proposed changes

    @1st1
    Copy link
    Member

    1st1 commented Oct 3, 2018

    [Andrew]

    I vote on not changing run_in_executor behavior if we cannot make it work with ProcessPoolExecutor.

    If a new API will solve the problem -- that's fine.
    Until it landed the explicit context propagation is the satisfactory solution.

    I'm not sure I understand. Should we close this issue or you're OK with (3)?

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Oct 3, 2018

    I prefer (4) but can live with (3)

    It is a new feature for Python 3.8 anyway, no need to rush

    @1st1
    Copy link
    Member

    1st1 commented Oct 3, 2018

    It is a new feature for Python 3.8 anyway, no need to rush

    Yep, I agree. Let's see if we end up having a new nice high-level API in 3.8; if not we go for (3).

    @1st1 1st1 added type-feature A feature request or enhancement and removed 3.7 (EOL) end of life labels Oct 3, 2018
    @hellysmile
    Copy link
    Mannequin Author

    hellysmile mannequin commented Aug 25, 2019

    Hey, as I see there is no new API yet.

    Can we take a look again on 3) ?

    I'll be happy to update PR 9688

    @csabella csabella added 3.9 only security fixes and removed 3.8 only security fixes labels Oct 20, 2019
    @asvetlov
    Copy link
    Contributor

    contextvars and run_in_executor() cannot be used together with process executor.

    asyncio.to_thread() runs with a copy of the current context.
    Available since Python 3.9

    https://docs.python.org/3/library/asyncio-task.html?highlight=to_thread#asyncio.to_thread

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants