Skip to content

gh-107149: Make PyUnstable_ExecutableKinds public #108440

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

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 24, 2023

Move PyUnstable_ExecutableKinds and associated macros from the internal C API to the public C API.

@vstinner
Copy link
Member Author

Since last month, nobody replied to my question if PyUnstable_ExecutableKinds should be made public on purpose, or if this PyUnstable API should stay in the internal C API. If it should stay private, why calling it this way?

PyUnstable_ExecutableKinds was added by @markshannon in commit 7199584.

#define PY_EXECUTABLE_KIND_SKIP 0
#define PY_EXECUTABLE_KIND_PY_FUNCTION 1
#define PY_EXECUTABLE_KIND_BUILTIN_FUNCTION 3
#define PY_EXECUTABLE_KIND_METHOD_DESCRIPTOR 4
#define PY_EXECUTABLE_KINDS 5

Hum, should constants also be prefixed by PyUnstable_ if they are exposed in the public C API? Maybe they can keep this name in the internal C API.

cc @encukou

@vstinner
Copy link
Member Author

@gvanrossum: Does this change look good to you? If this API is too new and should "not be used" outside CPython code base, maybe the PyUnstable API should be removed instead?

@vstinner
Copy link
Member Author

Note: this API is new in Python 3.13, Python 3.12 is not affected.

@gvanrossum
Copy link
Member

@gvanrossum: Does this change look good to you? If this API is too new and should "not be used" outside CPython code base, maybe the PyUnstable API should be removed instead?

I defer to @iritkatriel. Maybe it should have just been a private API, i.e. not using Unstable in its name?

@iritkatriel
Copy link
Member

Maybe it should have just been a private API, i.e. not using Unstable in its name?

Possibly. @markshannon do you have a preference?

@vstinner
Copy link
Member Author

  • Defining PyUnstable API in Include/cpython/ means that it's useful and relevant to use this API outside CPython code base.
  • Private functions (_Py prefix) in Include/internals/ means that for that for now, it's better to not allow 3rd party code to use it.

Make your choice :-)

@iritkatriel
Copy link
Member

If Mark doesn't have a different view, I'd make them private since that's easier to reverse.

@gvanrossum
Copy link
Member

Let's not waste Mark's time. Let's make it private.

@vstinner
Copy link
Member Author

Let's not waste Mark's time. Let's make it private.

Ok. I created PR #108651 to make the API private. Would you mind to review it? Thanks in advance.

I'd make them private since that's easier to reverse.

Sure, later, we can easily make it public if some use cases arise!

@gvanrossum
Copy link
Member

Hold on. I mixed up two PRs or issues of yours that both were related to PyUnstable. I see you already merged gh-108441 which is fine. Those are now public unstable.

For this PR I think the API also needs to remain or belong public/unstable, since this API is needed for 3rd parties that are inspecting frames. They'll find this executable kind code and need to be able to understand what it means.

So I retract my proposal/approval to make it private.

@vstinner
Copy link
Member Author

@markshannon: So is that your PR what you propose?

Move PyUnstable_ExecutableKinds and associated macros from the
internal C API to the public C API.

Rename constants: replace "PY_" prefix with "PyUnstable_" prefix.
@vstinner vstinner force-pushed the pyunstable_exec_kinds branch from cdf9ab3 to cc026df Compare August 30, 2023 11:49
@vstinner
Copy link
Member Author

I rebased the PR on the main branch and I renamed constants: replace PY_ prefix with PyUnstable_ prefix.

@vstinner
Copy link
Member Author

@markshannon wants this API to be public, see his comment: #108651 (comment)

@markshannon
Copy link
Member

markshannon commented Aug 30, 2023

The mixed case looks a bit strange. Is there a reason it can't be PY_UNSTABLE_...?
(or PYUNSTABLE_ if we don't want underscores in the prefix)

@vstinner
Copy link
Member Author

The mixed case looks a bit strange. Is there a reason it can't be PY_UNSTABLE_...?

cc @encukou who designed https://peps.python.org/pep-0689/

@vstinner
Copy link
Member Author

Most C API macros uses Py_ prefix (438), instead of the PY_ prefix (83):

$ cd ~/python/main/Include
$ git grep -E 'define Py_[A-Z_]+\>'|wc -l
438

$ git grep -E 'define PY_[A-Z_]+\>'|wc -l
83

I don't have a strong preference between PyUnstable_, PY_UNSTABLE_ or PYUNSTABLE_ prefix.

@encukou
Copy link
Member

encukou commented Aug 31, 2023

See PEP-7:

Macros should have a MixedCase prefix and then use upper case, for example: PyString_AS_STRING, Py_PRINT_RAW.

So, it should be PyUnstable_EXECUTABLE_KIND_SKIP &c.

@vstinner
Copy link
Member Author

See PEP-7: (...) So, it should be PyUnstable_EXECUTABLE_KIND_SKIP &c.

Oh ok. I prefer to make the doc even more explicit: #108710

@vstinner vstinner merged commit 9c03215 into python:main Aug 31, 2023
@vstinner vstinner deleted the pyunstable_exec_kinds branch August 31, 2023 07:56
@vstinner vstinner added the needs backport to 3.12 only security fixes label Aug 31, 2023
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 9c03215a3ee31462045b2c2ee162bdda30c54572 3.12

@vstinner vstinner removed the needs backport to 3.12 only security fixes label Aug 31, 2023
@vstinner
Copy link
Member Author

Note: this API is new in Python 3.13, Python 3.12 is not affected.

Oops, I forgot my own comment :-)

@vstinner
Copy link
Member Author

Thanks everybody for helping me to clarify the scope of this API :-) Thanks @encukou for helping me on the API prefix for constants (macros) :-) I merged my PR.

@bedevere-bot
Copy link

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

Hi! The buildbot ARM Raspbian 3.x has failed when building commit 9c03215.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/424/builds/4780) and take a look at the build logs.
  4. Check if the failure is related to this commit (9c03215) or if it is a false positive.
  5. 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/424/builds/4780

Failed tests:

  • test.test_concurrent_futures.test_shutdown

Failed subtests:

  • test_interpreter_shutdown - test.test_concurrent_futures.test_shutdown.ProcessPoolForkserverProcessPoolShutdownTest.test_interpreter_shutdown

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

== Tests result: FAILURE then FAILURE ==

442 tests OK.

10 slowest tests:

  • test_venv: 12 min 17 sec
  • test_largefile: 11 min 27 sec
  • test_posix: 5 min 16 sec
  • test_tarfile: 5 min 12 sec
  • test_math: 4 min 36 sec
  • test_support: 2 min 46 sec
  • test_zipimport_support: 2 min 37 sec
  • test.test_multiprocessing_spawn.test_processes: 2 min 30 sec
  • test_peg_generator: 2 min 14 sec
  • test_hashlib: 2 min 11 sec

1 test failed:
test.test_concurrent_futures.test_shutdown

19 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_idle
test_ioctl test_kqueue test_launcher test_perf_profiler
test_startfile test_tcl test_tkinter test_ttk test_ttk_textonly
test_turtle test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test.test_concurrent_futures.test_shutdown

Total duration: 42 min 35 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_concurrent_futures/test_shutdown.py", line 49, in test_interpreter_shutdown
    self.assertFalse(err)
AssertionError: b'Exception in thread Thread-1:\nTraceback (most recent call last):\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/threading.py", line 1059, in _bootstrap_inner\n    self.run()\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/process.py", line 339, in run\n    self.add_call_item_to_queue()\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/process.py", line 394, in add_call_item_to_queue\n    self.call_queue.put(_CallItem(work_id,\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/multiprocessing/queues.py", line 94, in put\n    self._start_thread()\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/multiprocessing/queues.py", line 177, in _start_thread\n    self._thread.start()\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/threading.py", line 978, in start\n    _start_new_thread(self._bootstrap, ())\nRuntimeError: can\'t create new thread at interpreter shutdown\nTraceback (most recent call last):\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/multiprocessing/forkserver.py", line 274, in main\n    code = _serve_one(child_r, fds,\n           ^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/multiprocessing/forkserver.py", line 313, in _serve_one\n    code = spawn._main(child_r, parent_sentinel)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/multiprocessing/spawn.py", line 132, in _main\n    self = reduction.pickle.load(from_parent)\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n  File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/multiprocessing/synchronize.py", line 115, in __setstate__\n    self._semlock = _multiprocessing.SemLock._rebuild(*state)\n                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\nFileNotFoundError: [Errno 2] No such file or directory\n' is not false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants