gh-108314: Add PyDict_ContainsString() function by vstinner · Pull Request #108323 · python/cpython

vstinner · 2023-08-22T17:05:50Z

Use PyDict_ContainsString() in pylifecycle.c and pythonrun.c.

Issue: [C API] Add PyDict_ContainsString() function #108314

📚 Documentation preview 📚: https://cpython-previews--108323.org.readthedocs.build/

methane

LGTM except one minor comment.

Python/pythonrun.c

serhiy-storchaka

CPython itself does not need this function, because _Py_ID() is used everywhere. But for third-party code it should come in handy.

serhiy-storchaka · 2023-08-23T06:03:57Z

Doc/c-api/dict.rst


   Insert *val* into the dictionary *p* using *key* as a key. *key* should
-   be a :c:expr:`const char*`.  The key object is created using
+   be a :c:expr:`const char*` UTF-8 encoded bytes string.  The key object is created using


It would be nice to backport such docs changes. Could you create a separate PR for this to make backporting easier? There may be other functions with const char * parameters without explicitly specified encoding.

Once this PR is merged, I will extract the doc change into a separated PR to backport these doc changes.

I created PR #108448 for the doc changes.

Lib/test/test_capi/test_dict.py

Include/dictobject.h

encukou · 2023-08-23T07:17:34Z

As in #100100, I still don't think this function is worth it. Not every two-liner (well, five-liner in C) needs to be exposed as API.

serhiy-storchaka · 2023-08-23T08:17:11Z

I think it is worth it, because attractive alternatives PyDict_GetItemString() and PyMapping_HasKeyString() have flaws.

encukou · 2023-08-23T09:58:24Z

Can we at least not add to the stable ABI, at least until we have a clearer idea of where the C API is going?

* The new function is not part of the limited C API. * Use PyDict_ContainsString() in pylifecycle.c and pythonrun.c.

vstinner · 2023-08-23T13:15:25Z

I updated my PR:

Don't add the function to the limited C API
Fix indentation
Add test on invalid UTF-8 string
PR rebased on the main branch

vstinner · 2023-08-23T13:20:13Z

@serhiy-storchaka:

CPython itself does not need this function, because _Py_ID() is used everywhere. But for third-party code it should come in handy.

Sadly yes, CPython has a bias: because CPython has nice tools like Argument Clinic and _Py_ID(), we are less exposed to the annoyance/limitation of the C API.

I hesitated to just use _Py_ID() in the 3 functions that I modified to use PyDict_ContainsString(), but these code paths are not performance critical and I was surprised that there was a "gap" in the API.

I was trying to replace _PyDict_GetItemStringWithError() with PyDict_GetItemStringRef() and I saw that PyDict_GetItemStringRef() also has its annoyance for such code path, since it requires adding Py_DECREF().

vstinner · 2023-08-23T13:24:31Z

Removing private functions from the public C API (issue #106320) is a way to discover gaps in the public C API. I also think that we should try to restrict ourselves to the public C API (or even the limited C API) in some/most C extensions of the stdlib to discover such issues, and complete the C API if needed.

Private functions like _PyImport_GetModuleAttr() and _PyImport_GetModuleAttrString() are easy to reimplement: it's just PyImport_ImportModule() + PyObject_GetAttr() (and + PyUnicode_FromString() for the String variant), but it's annoying to maintain your own implementation. Where is the limit between "useful recipe of 5-10 lines" and "a public documented and tested API"? I suppose that it should be discussed on a case by case basis.

vstinner · 2023-08-23T13:50:13Z

test_ssl failed on macOS with:

test_preauth_data_to_tls_client (test.test_ssl.TestPreHandshakeClose.test_preauth_data_to_tls_client) ... skipped "Could not recreate conditions on darwin: err=OSError(22, 'Invalid argument')"
Warning -- threading_cleanup() failed to cleanup 0 threads (count: 0, dangling: 2)
Warning -- Dangling thread: <SingleConnectionTestServerThread(preauth_data_to_tls_client, stopped 123145377214464)>
Warning -- Dangling thread: <_MainThread(MainThread, started 4495689216)>

vstinner · 2023-08-23T14:03:27Z

I prepared a PR to add the function to pythoncapi-compat: python/pythoncapi-compat#71

encukou · 2023-08-23T18:19:57Z

Where is the limit between "useful recipe of 5-10 lines" and "a public documented and tested API"

If we go with Mark's proposal of splitting things into a stable, performant, minimal set of “ABI” functions and an ergonomic C API that calls them, these kinds of helpers should go in the latter only.

But will we go with some variation of that proposal? Hopefully we'll have a better idea in a few months :)

vstinner · 2023-08-23T19:07:40Z

test_ssl failed on macOS with: Warning -- Dangling thread: <SingleConnectionTestServerThread(preauth_data_to_tls_client, stopped 123145377214464)>

I'm working on a fix for that: PR #108344 (merged) and PR #108370.

vstinner · 2023-08-23T21:43:01Z

@methane @serhiy-storchaka @encukou: I updated my PR, it's no longer part of the limited C API. So what do you think?

serhiy-storchaka

LGTM.

bedevere-bot · 2023-08-24T14:17:24Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian PGO 3.x has failed when building commit 6726626.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/249/builds/6075) and take a look at the build logs.
Check if the failure is related to this commit (6726626) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/249/builds/6075

Failed tests:

test_venv

Failed subtests:

test_with_pip - test.test_venv.EnsurePipTest.test_with_pip

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

440 tests OK.

10 slowest tests:

test_concurrent_futures: 2 min 7 sec
test_signal: 1 min 13 sec
test.test_multiprocessing_spawn.test_processes: 37.9 sec
test.test_multiprocessing_forkserver.test_processes: 37.4 sec
test_math: 33.6 sec
test_io: 30.7 sec
test_imaplib: 30.3 sec
test_socket: 29.2 sec
test_xmlrpc: 27.6 sec
test_subprocess: 25.5 sec

1 test failed:
test_venv

15 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_gdb
test_ioctl test_kqueue test_launcher test_startfile test_tkinter
test_ttk test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test_venv

Total duration: 9 min 40 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 781, in test_with_pip
    self.do_test_with_pip(False)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 704, in do_test_with_pip
    with self.nicer_error():
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/contextlib.py", line 159, in __exit__
    self.gen.throw(value)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 773, in nicer_error
    self.fail(
AssertionError: Command '['/tmp/test_python_jb_o9fbp/tmpes26zoc8/bin/python', '-m', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.


Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__main__.py", line 5, in <module>
    sys.exit(ensurepip._main())
             ^^^^^^^^^^^^^^^^^
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__init__.py", line 284, in _main
    return _bootstrap(
           ^^^^^^^^^^^
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__init__.py", line 200, in _bootstrap
    return _run_pip([*args, *_PACKAGE_NAMES], additional_paths)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/ensurepip/__init__.py", line 101, in _run_pip
    return subprocess.run(cmd, check=True).returncode
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/tmp/test_python_jb_o9fbp/tmpes26zoc8/bin/python', '-W', 'ignore::DeprecationWarning', '-c', '\nimport runpy\nimport sys\nsys.path = [\'/tmp/test_python_jb_o9fbp/tmpl3b71vad/pip-23.2.1-py3-none-any.whl\'] + sys.path\nsys.argv[1:] = [\'install\', \'--no-cache-dir\', \'--no-index\', \'--find-links\', \'/tmp/test_python_jb_o9fbp/tmpl3b71vad\', \'--upgrade\', \'pip\']\nrunpy.run_module("pip", run_name="__main__", alter_sys=True)\n']' returned non-zero exit status 1.


Traceback (most recent call last):
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 769, in nicer_error
    yield
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 705, in do_test_with_pip
    self.run_with_capture(venv.create, self.env_dir,
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 87, in run_with_capture
    func(*args, **kwargs)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 469, in create
    builder.create(env_dir)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 76, in create
    self._setup_pip(context)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 359, in _setup_pip
    self._call_new_python(context, '-m', 'ensurepip', '--upgrade',
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 355, in _call_new_python
    subprocess.check_output(args, **kwargs)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/subprocess.py", line 466, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/tmp/test_python_jb_o9fbp/tmpes26zoc8/bin/python', '-m', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 1.


Traceback (most recent call last):
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 781, in test_with_pip
    self.do_test_with_pip(False)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 705, in do_test_with_pip
    self.run_with_capture(venv.create, self.env_dir,
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/test/test_venv.py", line 87, in run_with_capture
    func(*args, **kwargs)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 469, in create
    builder.create(env_dir)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 74, in create
    self.setup_python(context)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 301, in setup_python
    copier(context.env_exe, path, relative_symlinks_ok=True)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/venv/__init__.py", line 236, in symlink_or_copy
    shutil.copyfile(src, dst)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/shutil.py", line 273, in copyfile
    _fastcopy_sendfile(fsrc, fdst)
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/shutil.py", line 164, in _fastcopy_sendfile
    raise err from None
  File "/var/lib/buildbot/workers/enable-optimizations-bot/3.x.gps-debian-profile-opt.pgo/build/Lib/shutil.py", line 150, in _fastcopy_sendfile
    sent = os.sendfile(outfd, infd, offset, blocksize)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [Errno 28] No space left on device: '/tmp/tmpu_g685c0/bin/python' -> '/tmp/tmpu_g685c0/bin/python3.13'

vstinner requested review from a team, encukou, markshannon and methane as code owners August 22, 2023 17:05

bedevere-bot mentioned this pull request Aug 22, 2023

[C API] Add PyDict_ContainsString() function #108314

Closed

bedevere-bot added the awaiting core review label Aug 22, 2023

methane reviewed Aug 23, 2023

View reviewed changes

Python/pythonrun.c Outdated Show resolved Hide resolved

serhiy-storchaka self-requested a review August 23, 2023 05:54

serhiy-storchaka reviewed Aug 23, 2023

View reviewed changes

pythongh-108314: Add PyDict_ContainsString() function

4916d67

* The new function is not part of the limited C API. * Use PyDict_ContainsString() in pylifecycle.c and pythonrun.c.

vstinner force-pushed the contains_string branch from b142bf9 to 4916d67 Compare August 23, 2023 13:14

vstinner mentioned this pull request Aug 23, 2023

Add PyDict_ContainsString() function python/pythoncapi-compat#71

Merged

Use PyDict_ContainsString() in _ctypes

f0dd34a

serhiy-storchaka approved these changes Aug 24, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 24, 2023

Fix compiler warnings in _ctypes.c

05680bc

vstinner merged commit 6726626 into python:main Aug 24, 2023

vstinner deleted the contains_string branch August 24, 2023 13:59

bedevere-bot removed the awaiting merge label Aug 24, 2023

Uh oh!

Conversation

vstinner commented Aug 22, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

encukou commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Aug 23, 2023

Uh oh!

encukou commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

encukou commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

vstinner commented Aug 23, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 24, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented Aug 22, 2023 •

edited by github-actions bot

Loading

encukou commented Aug 23, 2023 •

edited

Loading