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

Implement PEP 560: Core support for typing module and generic types #76407

Closed
ilevkivskyi opened this issue Dec 5, 2017 · 28 comments
Closed

Implement PEP 560: Core support for typing module and generic types #76407

ilevkivskyi opened this issue Dec 5, 2017 · 28 comments
Assignees
Labels
3.7 interpreter-core stdlib type-feature

Comments

@ilevkivskyi
Copy link
Contributor

@ilevkivskyi ilevkivskyi commented Dec 5, 2017

BPO 32226
Nosy @gvanrossum, @ned-deily, @serhiy-storchaka, @ilevkivskyi, @xgid
PRs
  • #4732
  • #4906
  • #5098
  • #5212
  • 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 = 'https://github.com/ilevkivskyi'
    closed_at = <Date 2018-01-29.23:35:04.753>
    created_at = <Date 2017-12-05.20:36:48.434>
    labels = ['interpreter-core', 'type-feature', 'library', '3.7']
    title = 'Implement PEP 560: Core support for typing module and generic types'
    updated_at = <Date 2018-01-29.23:35:04.752>
    user = 'https://github.com/ilevkivskyi'

    bugs.python.org fields:

    activity = <Date 2018-01-29.23:35:04.752>
    actor = 'levkivskyi'
    assignee = 'levkivskyi'
    closed = True
    closed_date = <Date 2018-01-29.23:35:04.753>
    closer = 'levkivskyi'
    components = ['Interpreter Core', 'Library (Lib)']
    creation = <Date 2017-12-05.20:36:48.434>
    creator = 'levkivskyi'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32226
    keywords = ['patch']
    message_count = 28.0
    messages = ['307682', '308092', '308101', '308121', '308151', '308171', '308291', '308312', '308323', '308341', '308346', '308347', '308351', '309484', '309485', '309829', '309830', '309927', '310149', '310150', '310208', '310336', '311027', '311035', '311036', '311038', '311148', '311199']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'ned.deily', 'serhiy.storchaka', 'levkivskyi', 'xgdomingo']
    pr_nums = ['4732', '4906', '5098', '5212']
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue32226'
    versions = ['Python 3.7']

    @ilevkivskyi ilevkivskyi added the 3.7 label Dec 5, 2017
    @ilevkivskyi ilevkivskyi self-assigned this Dec 5, 2017
    @ilevkivskyi ilevkivskyi added interpreter-core stdlib type-feature labels Dec 5, 2017
    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Dec 5, 2017

    As discussed before, there will be two PRs. One for the core components, and the second one (large) for typing updates. I will open the first PR shortly.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 12, 2017

    As for __class_getitem__, why not implement type.__getitem__ instead of hacking PyObject_GetItem()?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 12, 2017

    For reference: PEP-560.

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Dec 12, 2017

    As for __class_getitem__, why not implement type.__getitem__ instead of hacking PyObject_GetItem()?

    This question was raised by Mark Shannon on python-dev. Actually, my initial implementation did exactly this, but I didn't like it for two reasons:

    1. dir(type) is already very long, I don't want to make it even longer.
    2. Some users may be surprised that although type.__getitem__ is defined, int[something] will rise "TypeError: type object is not subscriptable"

    Mark however disappeared since then, so I don't know what is his current opinion. I am however not as sure about this now. If there are some arguments for this, then I can revert to the older approach (implementing type.__getitem__).

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Dec 12, 2017

    Guido, what is your preference for the implementation of __class_getitem__: PyObject_GetItem (current one) or type.__getitem__ (old one)? Can we just go ahead with the version you like and then re-consider if some objections will appear? I am asking because you are going on vacation soon and there is a second part of implementation -- changes in typing (and someone needs to review them :-)

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 12, 2017

    I like the current approach (PyObject_GetItem). I agree with both of your reasons to prefer it.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 14, 2017

    This adds an overhead to every indexing.

    $ ./python -m perf timeit --compare-to=./python0 -s 'a = [1]' --duplicate=10000 'a[0]'
    python0: ..................... 15.3 ns +- 0.5 ns
    python: ..................... 16.1 ns +- 0.3 ns

    Mean +- std dev: [python0] 15.3 ns +- 0.5 ns -> [python] 16.1 ns +- 0.3 ns: 1.05x slower (+5%)

    $ ./python -m perf timeit --compare-to=./python0 -s 'd = {1: 2}' --duplicate=10000 'd[1]'
    python0: ..................... 17.6 ns +- 0.6 ns
    python: ..................... 18.4 ns +- 0.5 ns

    Mean +- std dev: [python0] 17.6 ns +- 0.6 ns -> [python] 18.4 ns +- 0.5 ns: 1.05x slower (+5%)

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 14, 2017

    This adds an overhead to every indexing.

    I'm not doubting your results, but I'm curious how that happened. The extra
    code in PyObject_GetItem is right before the point where it would otherwise
    raise TypeError -- I presume you benchmark never reached that point. Could
    it be that the compiler ends up rearranging the code in a way that is less
    optimal for the instruction cache?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 14, 2017

    I think this is a cause. The difference may be dependent on the compiler and platform.

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Dec 14, 2017

    New changeset 2b5fd1e by Ivan Levkivskyi in branch 'master':
    bpo-32226: Implementation of PEP-560 (core components) (bpo-4732)
    2b5fd1e

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Dec 14, 2017

    Interesting. I have noticed similar when I tried to add a fast loop proposed by Serhiy. It should save at least one pointer comparison for base class, but sometimes it actually led to slow-downs (although very small). How can one fight this kind of problems?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 14, 2017

    I think the best thing to do is not to panic! Also maybe PGO could help?

    On Dec 14, 2017 15:43, "Ivan Levkivskyi" <report@bugs.python.org> wrote:

    Ivan Levkivskyi <levkivskyi@gmail.com> added the comment:

    Interesting. I have noticed similar when I tried to add a fast loop
    proposed by Serhiy. It should save at least one pointer comparison for base
    class, but sometimes it actually led to slow-downs (although very small).
    How can one fight this kind of problems?

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue32226\>


    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 15, 2017

    It is very to get stable benchmarks with timings shorter than 1 ms, and even harder for timings shorter than 1 us.

    I wrote documentation to implement how to get more stable results:
    http://perf.readthedocs.io/en/latest/run_benchmark.html#how-to-get-reproductible-benchmark-results

    On Linux, using CPU isolation helps a lot, and perf uses automatically isolated CPUs.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 4, 2018

    New changeset ce5b0e9 by Serhiy Storchaka in branch 'master':
    bpo-32226: Make __class_getitem__ an automatic class method. (bpo-5098)
    ce5b0e9

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 4, 2018

    Please don't forgot to document this feature.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 11, 2018

    Can I help?

    I'm implementing the feature related to pickling classes, and want to implement pickling generic types as an example. But since PEP-560 will change all here, I need to wait this change.

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Jan 11, 2018

    Serhiy,

    I am sorry for a delay, I have recently moved to another country, this is why I have not much time. I will try to work on this weekend. Here are some points where you can be helpful:

    • Make a short documentation PR for __mro_entry__ and __base_subclass__ methods.
    • Document the __class_getitem__ C API calling convention (with motivation, many classes are implemented in C but are generic in nature, like numpy.ndarray) in PEP-560.

    (I know this might be not very interesting, but this will save time for me to focus only on typing part.)

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 14, 2018

    Sorry, I know very small about this module to writing the documentation.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 17, 2018

    Since the commit 45700fb, test_genericclass leaks references:
    ---
    vstinner@apu$ ./python -m test -R 3:3 test_genericclass -m test.test_genericclass.CAPITest.test_c_class
    Run tests sequentially
    0:00:00 load avg: 1.27 [1/1] test_genericclass
    beginning 6 repetitions
    123456
    ......
    test_genericclass leaked [2, 3, 2] memory blocks, sum=7
    test_genericclass failed

    1 test failed:
    test_genericclass

    Total duration: 107 ms
    Tests result: FAILURE
    ---

    The leak comes from _testcapi.Generic[int]. Maybe from generic_alias_new() of Modules/_testcapi.c.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 17, 2018

    Oh ok, I think that I found the bug: PR 5212.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 17, 2018

    New changeset e860089 by Victor Stinner in branch 'master':
    bpo-32226: Fix memory leak in generic_alias_dealloc() (bpo-5212)
    e860089

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Jan 20, 2018

    New changeset d911e40 by Ivan Levkivskyi in branch 'master':
    bpo-32226: PEP-560: improve typing module (bpo-4906)
    d911e40

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 28, 2018

    What's the status of this? The issue is still open with "release blocker" status.

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Jan 29, 2018

    I think all the critical things have been implemented/fixed. There are unfortunately no docs yet (my fault), also there are two bugs related to PEP-560, but they are not new, they also exist in Python 3.6.

    I am however marking this as high priority, since these two bugs prevent progress on other projects (pickling generic classes), so it would be good to fix them before beta-2.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 29, 2018

    Are there more specific descriptions of those bugs? Links to a tracker?
    (Even if it's the typing tracker.)

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Jan 29, 2018

    python/typing#512 and python/typing#511 are first issue (they are closely related to each other). This is not directly related to PEP-560, but changes in typing because of it will allow to fix them in a simple manner.

    The second one is Union simplification (was discussed in few PRs on typing tracker). This is not really a bug, but something we should decide on before 3.7 final. It will be difficult to change it later. Again, with PEP-560 in place the corresponding refactoring for removal should be straightforward.

    In addition, there is python/typing#392, this is already almost fixed by the typing PR implementing PEP-560, but it doesn't work for dunder attributes (I think we should not relay only specific set of reserved names, rather than all dunders).

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 29, 2018

    ISTM that those ought to be separate issues since PEP-560 has been
    implemented fully here.

    Also I am getting cold feet about Union simplification (esp. this late in
    the release cycle). We should probably retreat on the typing tracker and
    discuss why we're considering it -- is there evidence (even anecdotal) that
    it's harmful?

    @ilevkivskyi
    Copy link
    Contributor Author

    @ilevkivskyi ilevkivskyi commented Jan 29, 2018

    OK, I will close this issue, and open separate issues for documentation, Union, etc.

    @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.7 interpreter-core stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants