GeezerVison is showing github.com — gh-108314: Add PyDict_ContainsString() function by vstinner · Pull Request #108323 · python/cpython
Conversation
vstinner
commented
Aug 22, 2023
•
edited by github-actions
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except one minor comment.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CPython itself does not need this function, because _Py_ID() is used everywhere. But for third-party code it should come in handy.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
|
|
|
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this PR is merged, I will extract the doc change into a separated PR to backport these doc changes.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created PR #108448 for the doc changes.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
encukou
commented
Aug 23, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I think it is worth it, because attractive alternatives PyDict_GetItemString() and PyMapping_HasKeyString() have flaws.
|
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
* The new function is not part of the limited C API.
* Use PyDict_ContainsString() in pylifecycle.c and pythonrun.c.
|
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
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@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().
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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)>
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 :)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
⚠️⚠️⚠️ 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:
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'
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
serhiy-storchaka
left a comment