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

Jump to bottom

Use Argument Clinic in sqlite3 #85128

Closed
erlend-aasland opened this issue Jun 12, 2020 · 24 comments
Closed

Use Argument Clinic in sqlite3 #85128

erlend-aasland opened this issue Jun 12, 2020 · 24 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@erlend-aasland
Copy link
Contributor

BPO 40956
Nosy @vstinner, @berkerpeksag, @serhiy-storchaka, @corona10, @pablogsal, @erlend-aasland
PRs
  • bpo-40956: Use Argument Clinic in sqlite3 module #20826
  • bpo-40956: Convert _sqlite3 module level functions to Argument Clinic #22484
  • bpo-41861: Convert _sqlite3 CursorType and ConnectionType to heap types #22478
  • bpo-40956: Convert _sqlite3.Connection to Argument Clinic #23057
  • bpo-40956: Convert _sqlite3.Connection to Argument Clinic, part 1 of 2 #23341
  • bpo-40956: Fix sqlite3 AC code #23837
  • bpo-40956: Convert _sqlite3.Connection to Argument Clinic, part 2 #23838
  • bpo-40956: Convert _sqlite3.Row to Argument Clinic #23964
  • bpo-40956: Convert _sqlite3.Cursor to Argument Clinic #24007
  • bpo-40956: Convert _sqlite3.Cache to Argument Clinic #24135
  • bpo-40956: fix sqlite3.Cursor.fetchmany() default value #24214
  • bpo-40956: Convert sqlite3.connect and sqlite3.Connection.__init__ to AC #24421
  • bpo-40956: [regression] first argument in sqlite3.Connection.backup is mandatory #24503
    		•

    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 2021-06-20.20:29:03.608>
created_at = <Date 2020-06-12.10:05:35.702>
labels = ['type-feature', 'library', '3.10']
title = 'Use Argument Clinic in sqlite3'
updated_at = <Date 2021-06-20.20:29:03.605>
user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2021-06-20.20:29:03.605>
actor = 'erlendaasland'
assignee = 'none'
closed = True
closed_date = <Date 2021-06-20.20:29:03.608>
closer = 'erlendaasland'
components = ['Library (Lib)']
creation = <Date 2020-06-12.10:05:35.702>
creator = 'erlendaasland'
dependencies = []
files = []
hgrepos = []
issue_num = 40956
keywords = ['patch']
message_count = 24.0
messages = ['371347', '378558', '378560', '378561', '378562', '378571', '380042', '380043', '380047', '383301', '383303', '383841', '384000', '384444', '385012', '385015', '385019', '385036', '385053', '385058', '386814', '396188', '396191', '396197']
nosy_count = 6.0
nosy_names = ['vstinner', 'berker.peksag', 'serhiy.storchaka', 'corona10', 'pablogsal', 'erlendaasland']
pr_nums = ['20826', '22484', '22478', '23057', '23341', '23837', '23838', '23964', '24007', '24135', '24214', '24421', '24503']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue40956'
versions = ['Python 3.10']
    @erlend-aasland
    Copy link
    Contributor Author

    Use Argument Clinic in sqlite3.

    @erlend-aasland erlend-aasland added 3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jun 12, 2020
    @corona10
    Copy link
    Member

    What's the purpose of using AC, did the change improve performance?
    It can make hard to track the code history.

    @erlend-aasland
    Copy link
    Contributor Author

    The primary reason is that it will be provide easy access to module state.

    The first step in making sqlite3 support multiphase init was to create heap types. The second step is argument clinic. The third will be to use AC for module state. The last step will then be final multiphase support.

    Also, IMHO, AC greatly improves the body of methods (readability => maintainability, hardened parsing of arguments)

    @erlend-aasland
    Copy link
    Contributor Author

    Also, see the comment from Victor here: #22478 (comment)

    @corona10
    Copy link
    Member

    Also, see the comment from Victor here: #22478 (comment)

    Okay got it

    @erlend-aasland
    Copy link
    Contributor Author

    By the way, what's the preferred way to benchmark performance?

    @corona10
    Copy link
    Member

    New changeset 7d21027 by Erlend Egeberg Aasland in branch 'master':
    bpo-40956: Convert _sqlite3 module level functions to Argument Clinic (GH-22484)
    7d21027

    @corona10
    Copy link
    Member

    @erlendaasland

    Thank you erlendaasland for working on #66674.
    This time was a good time to review my AC knowledge ;)

    @erlend-aasland
    Copy link
    Contributor Author

    Thank you erlendaasland for working on #66674.

    … and thank you for taking the time to review this!

    @corona10
    Copy link
    Member

    New changeset 1ba82bb by Erlend Egeberg Aasland in branch 'master':
    bpo-40956: Convert _sqlite3.Connection to Argument Clinic (GH-23341)
    1ba82bb

    @corona10
    Copy link
    Member

    New changeset 2179349 by Dong-hee Na in branch 'master':
    bpo-40956: Fix sqlite3 AC code (GH-23837)
    2179349

    @corona10
    Copy link
    Member

    New changeset 3ccef1c by Erlend Egeberg Aasland in branch 'master':
    bpo-40956: Convert _sqlite3.Connection to Argument Clinic, part 2 (GH-23838)
    3ccef1c

    @serhiy-storchaka
    Copy link
    Member

    New changeset 84d79cf by Erlend Egeberg Aasland in branch 'master':
    bpo-40956: Convert _sqlite3.Row to Argument Clinic (GH-23964)
    84d79cf

    @berkerpeksag
    Copy link
    Member

    New changeset c7f8d3c by Erlend Egeberg Aasland in branch 'master':
    bpo-40956: Convert _sqlite3.Cursor to Argument Clinic (GH-24007)
    c7f8d3c

    @erlend-aasland
    Copy link
    Contributor Author

    #68195 introduced a regression:

    >>> import sqlite3
>>> help(sqlite3)
Traceback (most recent call last):
  File "Lib/inspect.py", line 2049, in wrap_value
    value = eval(s, module_dict)
  File "<string>", line 1, in <module>
NameError: name 'cursor' is not defined

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "Lib/inspect.py", line 2052, in wrap_value
    value = eval(s, sys_module_dict)
  File "<string>", line 1, in <module>
NameError: name 'cursor' is not defined

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "Lib/_sitebuiltins.py", line 103, in __call__
    return pydoc.help(*args, **kwds)
  File "Lib/pydoc.py", line 2000, in __call__
    self.help(request)
  File "Lib/pydoc.py", line 2059, in help
    else: doc(request, 'Help on %s:', output=self._output)
  File "Lib/pydoc.py", line 1779, in doc
    pager(render_doc(thing, title, forceload))
  File "Lib/pydoc.py", line 1772, in render_doc
    return title % desc + '\n\n' + renderer.document(object, name)
  File "Lib/pydoc.py", line 472, in document
    if inspect.ismodule(object): return self.docmodule(*args)
  File "Lib/pydoc.py", line 1267, in docmodule
    contents.append(self.document(value, key, name))
  File "Lib/pydoc.py", line 473, in document
    if inspect.isclass(object): return self.docclass(*args)
  File "Lib/pydoc.py", line 1433, in docclass
    attrs = spill("Methods %s:\n" % tag, attrs,
  File "Lib/pydoc.py", line 1382, in spill
    push(self.document(value,
  File "Lib/pydoc.py", line 474, in document
    if inspect.isroutine(object): return self.docroutine(*args)
  File "Lib/pydoc.py", line 1492, in docroutine
    signature = inspect.signature(object)
  File "Lib/inspect.py", line 3129, in signature
    return Signature.from_callable(obj, follow_wrapped=follow_wrapped,
  File "Lib/inspect.py", line 2877, in from_callable
    return _signature_from_callable(obj, sigcls=cls,
  File "Lib/inspect.py", line 2351, in _signature_from_callable
    return _signature_from_builtin(sigcls, obj,
  File "Lib/inspect.py", line 2164, in _signature_from_builtin
    return _signature_fromstr(cls, func, s, skip_bound_arg)
  File "Lib/inspect.py", line 2102, in _signature_fromstr
    p(name, default)
  File "Lib/inspect.py", line 2084, in p
    default_node = RewriteSymbolics().visit(default_node)
  File "Lib/ast.py", line 410, in visit
    return visitor(node)
  File "Lib/inspect.py", line 2071, in visit_Attribute
    return wrap_value(value)
  File "Lib/inspect.py", line 2054, in wrap_value
    raise RuntimeError()
RuntimeError

    @erlend-aasland
    Copy link
    Contributor Author

    Looks like this is the culprit:

    cpython/Modules/_sqlite/cursor.c

    Line 825 in 2396614

    size as maxrows: int(c_default='self->arraysize') = cursor.arraysize

    @erlend-aasland
    Copy link
    Contributor Author

    I don't see how to easily solve this in AC: we want the arraysize of the cursor instance, but I don't see how I'm going to get this via the AC namespace/context. Using a "bogus" default value works:

    size as maxrows: int(c_default='self->arraysize') = 1

    Is there a better way?

    @serhiy-storchaka
    Copy link
    Member

    Use NULL.

    @erlend-aasland
    Copy link
    Contributor Author

    Thanks, but I prefer the following:

    size as maxrows: int(c_default='self->arraysize', py_default='<unrepresentable>') = 1

    @berkerpeksag
    Copy link
    Member

    New changeset a330365 by Erlend Egeberg Aasland in branch 'master':
    bpo-40956: Fix sqlite3.Cursor.fetchmany() default value (GH-24214)
    a330365

    @berkerpeksag
    Copy link
    Member

    New changeset ea46579 by Erlend Egeberg Aasland in branch 'master':
    bpo-40956: Fix segfault when Connection.backup is called without target (GH-24503)
    ea46579

    @pablogsal
    Copy link
    Member

    New changeset 185ecdc by Erlend Egeberg Aasland in branch 'main':
    bpo-40956: Convert sqlite3.connect and sqlite3.Connection.__init__ to AC (GH-24421)
    185ecdc

    @pablogsal
    Copy link
    Member

    Erlend, is anything left in this issue?

    @erlend-aasland
    Copy link
    Contributor Author

    Erlend, is anything left in this issue?

    Nothing left; thank you for your guidance and reviews, Dong-hee, Berker, Serhiy, and Pablo.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @erlend-aasland erlend-aasland mentioned this issue May 21, 2022
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @berkerpeksag @serhiy-storchaka @corona10 @pablogsal @erlend-aasland