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

Deprecate explicit loop parameter in all public asyncio APIs #80554

Closed
dtrauma mannequin opened this issue Mar 19, 2019 · 29 comments
Closed

Deprecate explicit loop parameter in all public asyncio APIs #80554

dtrauma mannequin opened this issue Mar 19, 2019 · 29 comments
Labels
3.8 only security fixes docs Documentation in the Doc dir topic-asyncio type-feature A feature request or enhancement

Comments

@dtrauma
Copy link
Mannequin

dtrauma mannequin commented Mar 19, 2019

BPO 36373
Nosy @asvetlov, @1st1, @Carreau, @eamanu, @miss-islington, @tirkarthi
PRs
  • bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [task] [WIP] #13670
  • bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [streams] #13671
  • [3.8] bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [streams] (GH-13671) #13833
  • bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [locks] #13920
  • bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [queue] #13950
  • [3.8] bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [locks] (GH-13920) #15835
  • [3.8] bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [queue] (GH-13950) #15841
  • bpo-36373: Fix deprecation warnings #15889
  • [3.8] bpo-36373: Fix deprecation warnings (GH-15889) #15896
  • [3.8] bpo-36373: Fix deprecation warnings (GH-15889) #15901
  • bpo-36373: Deprecate explicit loop in task and subprocess API #16033
  • [3.8] bpo-36373: Deprecate explicit loop in task and subprocess API (GH-16033) #16039
  • 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 2019-09-12.14:22:26.970>
    created_at = <Date 2019-03-19.19:55:35.538>
    labels = ['type-feature', '3.8', 'docs', 'expert-asyncio']
    title = 'Deprecate explicit loop parameter in all public asyncio APIs'
    updated_at = <Date 2019-09-12.14:22:26.970>
    user = 'https://bugs.python.org/dtrauma'

    bugs.python.org fields:

    activity = <Date 2019-09-12.14:22:26.970>
    actor = 'asvetlov'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2019-09-12.14:22:26.970>
    closer = 'asvetlov'
    components = ['Documentation', 'asyncio']
    creation = <Date 2019-03-19.19:55:35.538>
    creator = 'dtrauma'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 36373
    keywords = ['patch']
    message_count = 29.0
    messages = ['338406', '338452', '338984', '338985', '338986', '343809', '343810', '343882', '343887', '343888', '343889', '344673', '344674', '351618', '351624', '351626', '351630', '351651', '351658', '351665', '351677', '351679', '351684', '351729', '351739', '351770', '351805', '352145', '352150']
    nosy_count = 8.0
    nosy_names = ['asvetlov', 'docs@python', 'yselivanov', 'mbussonn', 'eamanu', 'miss-islington', 'xtreak', 'dtrauma']
    pr_nums = ['13670', '13671', '13833', '13920', '13950', '15835', '15841', '15889', '15896', '15901', '16033', '16039']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue36373'
    versions = ['Python 3.8']

    @dtrauma
    Copy link
    Mannequin Author

    dtrauma mannequin commented Mar 19, 2019

    https://docs.python.org/3.7/library/asyncio-task.html#running-tasks-concurrently

    For asyncio.gather, the docs should probably say

    The loop argument is deprecated and scheduled for removal in Python 3.10.

    as they do for all the other loop arguments of other functions.

    @dtrauma dtrauma mannequin added the 3.7 (EOL) end of life label Mar 19, 2019
    @dtrauma dtrauma mannequin assigned docspython Mar 19, 2019
    @dtrauma dtrauma mannequin added docs Documentation in the Doc dir type-feature A feature request or enhancement labels Mar 19, 2019
    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented Mar 20, 2019

    Hi,

    Reading the asyncio.gather code not seem to be deprecrated.
    loop is used on a lot of line of code.

    Maybe, the deprecate idea is on other place where I don't know.

    @dtrauma
    Copy link
    Mannequin Author

    dtrauma mannequin commented Mar 27, 2019

    Just to be clear, I don't know if loop is deprecated on this function like on all the others, I just suspect it to be. But it currently is completely undocumented, which either way is a bug. :)

    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented Mar 27, 2019

    @dtrauma jus for clarify. You say that if loop is not deprecated document it else document it.

    Right?

    @dtrauma
    Copy link
    Mannequin Author

    dtrauma mannequin commented Mar 27, 2019

    Yes, exactly, document deprecation status XOR what it does :)

    @asvetlov
    Copy link
    Contributor

    *Passing* loop to gather should be deprecated.

    *Using* loop by *internal logic* is pretty fine, we do it in asyncio everywhere.

    Yuri, I think we should deprecate explicit loop everywhere in non-deprecated asyncio API by Python 3.8.

    We can do it even in Python beta I think.

    @1st1
    Copy link
    Member

    1st1 commented May 28, 2019

    Yuri, I think we should deprecate explicit loop everywhere in non-deprecated asyncio API by Python 3.8.

    +1

    @asvetlov
    Copy link
    Contributor

    A champion is welcome :)

    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented May 29, 2019

    hello, I will work on it, if there are no objection. :-)

    @asvetlov
    Copy link
    Contributor

    Thank you.

    The change is pretty straightforward.

    There is no need to jumbo PR, you can split the work into PR-per-module if it is more comfortable to you.

    I'm ok with reviewing any approach.

    @asvetlov asvetlov changed the title asyncio.gather: no docs for deprecated loop parameter Deprecate explicit loop parameter in all public asyncio APIs May 29, 2019
    @asvetlov asvetlov added 3.8 only security fixes and removed 3.7 (EOL) end of life labels May 29, 2019
    @eamanu
    Copy link
    Mannequin

    eamanu mannequin commented May 29, 2019

    Hi

    There is no need to jumbo PR, you can split the work into PR-per-module if it is more comfortable to you.
    Yes, I think that is better split I will try it.
    I'm ok with reviewing any approach.
    Thanks!

    @eamanu eamanu mannequin changed the title Deprecate explicit loop parameter in all public asyncio APIs asyncio.gather: no docs for deprecated loop parameter May 29, 2019
    @asvetlov asvetlov changed the title asyncio.gather: no docs for deprecated loop parameter Deprecate explicit loop parameter in all public asyncio APIs May 29, 2019
    @miss-islington
    Copy link
    Contributor

    New changeset 6d64a8f by Miss Islington (bot) (Emmanuel Arias) in branch 'master':
    bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [streams] (GH-13671)
    6d64a8f

    @miss-islington
    Copy link
    Contributor

    New changeset 8899b11 by Miss Islington (bot) in branch '3.8':
    bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [streams] (GH-13671)
    8899b11

    @miss-islington
    Copy link
    Contributor

    New changeset 537877d by Miss Islington (bot) (Emmanuel Arias) in branch 'master':
    bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [locks] (GH-13920)
    537877d

    @miss-islington
    Copy link
    Contributor

    New changeset bb8fc8b by Miss Islington (bot) in branch '3.8':
    bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [locks] (GH-13920)
    bb8fc8b

    @miss-islington
    Copy link
    Contributor

    New changeset 9008be3 by Miss Islington (bot) (Emmanuel Arias) in branch 'master':
    bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [queue] (GH-13950)
    9008be3

    @miss-islington
    Copy link
    Contributor

    New changeset 55daf1a by Miss Islington (bot) in branch '3.8':
    bpo-36373: Deprecate explicit loop parameter in all public asyncio APIs [queue] (GH-13950)
    55daf1a

    @tirkarthi
    Copy link
    Member

    async_case passes loop parameter to the queue used to collect tests and this causes below DeprecationWarning with last change over deprecation in queues and locks API.

    ./python.exe -m unittest Lib.unittest.test.test_async_case
    /Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/async_case.py:116: DeprecationWarning: The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.
    self._asyncioCallsQueue = asyncio.Queue(loop=loop)
    /Users/karthikeyansingaravelan/stuff/python/cpython/Lib/asyncio/queues.py:48: DeprecationWarning: The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.
    self._finished = locks.Event(loop=self._loop)
    .......
    ----------------------------------------------------------------------
    Ran 7 tests in 0.083s

    OK

    @tirkarthi
    Copy link
    Member

    Few more tests and call sites

    ./python.exe -Wall -m test test_asyncio
    Run tests sequentially
    0:00:00 load avg: 2.41 [1/1] test_asyncio
    /Users/karthikeyansingaravelan/stuff/python/cpython/Lib/asyncio/locks.py:335: DeprecationWarning: The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.
    lock = Lock(loop=self._loop)
    test_asyncio passed in 1 min 38 sec

    == Tests result: SUCCESS ==

    1 test OK.

    Total duration: 1 min 38 sec
    Tests result: SUCCESS

    call sites

    >>> list(asyncio.tasks.as_completed([asyncio.ensure_future(asyncio.sleep(1))]))
    /Users/karthikeyansingaravelan/stuff/python/cpython/Lib/asyncio/tasks.py:579: DeprecationWarning: The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.
      done = Queue(loop=loop)
    /Users/karthikeyansingaravelan/stuff/python/cpython/Lib/asyncio/queues.py:48: DeprecationWarning: The loop argument is deprecated since Python 3.8, and scheduled for removal in Python 3.10.
      self._finished = locks.Event(loop=self._loop)

    @asvetlov
    Copy link
    Contributor

    Thanks for the report xtreak, I'll make a fix.

    @tirkarthi
    Copy link
    Member

    Is it okay to have _asyncio_internal keyword with default value as False and the places where it's used internally in stdlib like AsyncMock (Condition), async_case (Queue) and other tests we can pass in True? I have a patch if that would be a good approach :)

    @asvetlov
    Copy link
    Contributor

    Better to avoid _asyncio_internal if not strictly necessary.
    Currently, _asyncio_internal is used for protection of asyncio class direct instantiation.

    E.g. stream = asyncio.Stream() is forbidden, people should use factories like stream = await asyncio.connect(...) for it.

    @tirkarthi
    Copy link
    Member

    Ah okay. There were places like Condition's constructor that has a deprecation warning and we construct a Lock with loop passed in the constructor and I passed along _asyncio_internal. I would be happy to look into your PR since mine is just passing along _asyncio_internal=True for all the stdlib calls.

    I think it would be good if tests ran with warnings (at least DeprecationWarning and some class of warnings) as error but not sure why :(

    @1st1
    Copy link
    Member

    1st1 commented Sep 10, 2019

    (copying my GitHub message on this topic from #15889 (comment))

    I thought we were going to be more subtle about this.

    We have two scenarios:

    1. Somebody creates an instance of asyncio.Lock outside of a coroutine, also manually creating the loop, etc. In this case the user has to pass the lock argument.

    2. Somebody creates an instance of asyncio.Lock in a coroutine (that is, wheh asyncio.get_running_loop() returns a loop). In this case passing the loop argument is an error.

    The (1) approach is how people were used to writing asyncio programs before asyncio.run() (that was the only way actually).

    The (2) approach is how we want people to write asyncio programs.

    There's a subtle difference between things like asyncio.gather() and asyncio.Lock. Passing loop to the former is just nonsensical. Passing loop to the latter can be a valid thing, it's the (1).

    If we remove the loop parameter entirely from classes like asyncio.Lock we're making (1) impossible. I'm not sure that is what we are ready to do now.

    @asvetlov thoughts?

    @asvetlov
    Copy link
    Contributor

    Answered in #15889

    @miss-islington
    Copy link
    Contributor

    New changeset 7264e92 by Miss Islington (bot) (Andrew Svetlov) in branch 'master':
    bpo-36373: Fix deprecation warnings (GH-15889)
    7264e92

    @asvetlov
    Copy link
    Contributor

    New changeset 4601f7a by Andrew Svetlov in branch '3.8':
    [3.8] bpo-36373: Fix deprecation warnings (GH-15889) (GH-15901)
    4601f7a

    @asvetlov
    Copy link
    Contributor

    New changeset a488879 by Andrew Svetlov in branch 'master':
    bpo-36373: Deprecate explicit loop in task and subprocess API (GH-16033)
    a488879

    @miss-islington
    Copy link
    Contributor

    New changeset 345bfc9 by Miss Islington (bot) in branch '3.8':
    bpo-36373: Deprecate explicit loop in task and subprocess API (GH-16033)
    345bfc9

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes docs Documentation in the Doc dir topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants