scoped_interpreter. overloaded constructor: PyConfig param #4330

arman-novikov · 2022-11-13T10:37:10Z

Description

In some cases it is convenient to have an opportunity to construct a scoped_interpreter with a custom PyConfig instance supplied.

PyConfig my_config{};
//...
pybind11::scoped_interpreter{my_config}

Suggested changelog entry:

* feat: scoped_interpreter overloaded ctor: PyConfig param
* tests: Add program dir to path, Custom PyConfig, Custom PyConfig with argv

Skylion007

Also, it would be great to have a test added to our test suite as well.

Skylion007 · 2022-11-13T17:48:23Z

include/pybind11/embed.h

        PyStatus status = Py_InitializeFromConfig(&config);
        if (PyStatus_Exception(status)) {
-            throw std::runtime_error(PyStatus_IsError(status) ? status.err_msg : "Failed to init CPython");
+            throw std::runtime_error(PyStatus_IsError(status) ? status.err_msg
+                                                              : "Failed to init CPython");
        }


This code appears to be entirely duplicated from the end on initialize. Furthermore, for consistency py::initialize should also be able to take a config on recent python versions. Therefore, I would suggest abstracting this into an overload for initialize_interpreter(const PyConfig & config) that is only built when the python is recent enough. That would reduce code duplicate and make this a bit cleaner without changing the API that much.

The scoped_interepreter is really just an RAII for these calls anyway.

Thank you for your feedback. I find your suggestions reasonable.

However, I would rather get a sign of approval from the maintainers that they [want/do not mind] to have the feature introduced into the project before I rush into refactoring and extending tests. Is it at all possible to find out their opinion?

Seem reasonable to me, @rwgk @henryiii thoughts?

rwgk · 2022-11-13T20:49:26Z

I think it'll be great to support passing PyConfig, and ideally to clean up:

The current implementation of initialize_interpreter() was merged in a rush despite me being "really concerned": #3923 (comment)

EDIT OOPS this is NOT TRUE, I just made a mistake:

I stopped looking when I saw the merge commit back then while working on my feedback, but looking again now, I see that the argc and argv arguments are completely and silently ignored in the Python 3.11 branch, which is obviously error prone, especially because the function documentation was not updated when the Python 3.11 branch was added.

I think a good start for this PR would be to cleanly split the two implementations, ~~which will make the argc / argv situation immediately obvious, then to figure out how to best design passing PyConfig~~

without duplicating code, and
~~without silently (runtime or documentation) ignoring arguments, depending on the Python version.~~

It's not immediately obvious to me how to provide an API that works intuitively for Python < 3.11 and >= 3.11. Can it be done in a reasonable way without expecting

#if PY_VERSION_HEX < 0x030B0000
...
#else
...
#endif

constructs in user code? @arman-novikov what do you think?

rwgk · 2022-11-13T21:18:45Z

Sorry our codesearch hover-over tooling failed me: it doesn't highlight the argc, argv in the 3.11 branch for some reason (I just double-checked). I only realized when searching in a normal editor.

That changes my thinking a lot:

Now I'm thinking we need all arguments for >= 3.11, rough start:

    explicit scoped_interpreter(bool init_signal_handlers = true,
                                int argc = 0,
                                const char *const *argv = nullptr,
                                bool add_program_dir_to_path = true
                                PyConfig *config = nullptr) {

Although init_signal_handlers is redundant if config is passed. Not sure how to best deal with that.

Also not sure what to do about PyConfig for < 3.11, maybe like this? (Note: that code was removed already when we dropped Python 2 support.)

pybind11/include/pybind11/pybind11.h

Lines 1234 to 1238 in ec24786

    
           #if PY_MAJOR_VERSION >= 3 
        
               using module_def = PyModuleDef; 
        
           #else 
        
               struct module_def {}; 
        
           #endif

rwgk

I really like the direction this is going.

rwgk · 2022-11-16T05:29:20Z

include/pybind11/embed.h

+    config_guard(PyConfig& config): m_config{config}{}
+    ~config_guard(){PyConfig_Clear(&m_config);}
+private:
+    PyConfig& m_config;


I don't know if that's behind the Python 3.11 segfaults in the CI, but it's very error prone. The performance hit for making this safe is probably not measurable in any real world use, or even unit tests.

If you really want to pass in an existing config:

Just copy.

Or use std::unique_ptr, which would also make this RAII pattern safe.

If not using std::unique_ptr, the copy ctor and operator need to be deleted.

BUT

scoped_config would seem more fitting as a name.

How about this?

struct scoped_config { scoped_config() = default; ~scoped_config() { PyConfig_Clear(&config); }; scoped_config(const scoped_config &) = delete; scoped_config &operator=(const scoped_config &) = delete; PyConfig config; }; ... scoped_config scoped_cfg; PyConfig_InitIsolatedConfig(&scoped_cfg.config); scoped_cfg.config.isolated = 0;

That is an excellent move. I will introduce it asap.

rwgk · 2022-11-16T05:29:23Z

include/pybind11/embed.h

+
+/*!
+ * \warning should not be called by user
+ * \todo put it into the scoped_interpreter's private section?


You could simply put this in the detail namespace, like a lot of existing code not intended for public use.

rwgk · 2022-11-16T05:32:59Z

include/pybind11/embed.h

-                               "if sys.argv and os.path.exists(sys.argv[0]) else '')");
-        }
+        precheck_interpreter();
+        _initialize_interpreter(config, add_program_dir_to_path);


Symbols with a leading underscore are reserved for the compiler + library.
In this codebase we don't have them anywhere AFAIK.
detail::initialize_interpreter() could work.

Skylion007 · 2022-11-16T16:01:30Z

include/pybind11/embed.h

+#if PY_VERSION_HEX >= 0x030B0000
+class config_guard {
+public:
+    config_guard(PyConfig &config) : m_config{config} {}


delete copy ctor, and copy assignment.

Skylion007 · 2022-11-16T16:01:43Z

include/pybind11/embed.h

@@ -88,6 +88,42 @@ inline wchar_t *widen_chars(const char *safe_arg) {

 PYBIND11_NAMESPACE_END(detail)

+inline void precheck_interpreter() {


This should DEFINITELY be in detail namespace as well.

rwgk · 2022-11-16T16:09:07Z

BTW

While looking at the documentation for PyConfig I noticed "New in version 3.8."

I'm wondering, would it make sense / should we try to change the condition to #if PY_VERSION_HEX >= 0x03080000?

But what I'd probably do:

get it to pass with 3.11 first,
only then try how far back we can go.

arman-novikov · 2022-11-18T22:46:19Z

@Skylion007, the test has been introduced in a commit. I was forced to set up a newer version of python (3.8) in CI. The CI change has been introduced in commit. Would you please review the change?

arman-novikov · 2022-11-18T22:49:15Z

BTW

While looking at the documentation for PyConfig I noticed "New in version 3.8."

I'm wondering, would it make sense / should we try to change the condition to #if PY_VERSION_HEX >= 0x03080000?

But what I'd probably do:
* get it to pass with 3.11 first,

* only then try how far back we can go.

It seems python 3.8 was the first version with PyConfig support.

rwgk · 2022-11-21T11:05:18Z

.appveyor.yml

@@ -9,7 +9,7 @@ platform:
 - x86
 environment:
  matrix:
-  - PYTHON: 36
+  - PYTHON: 38


I was forced to set up a newer version of python (3.8) in CI.

This sounds like a misunderstanding. Whatever is done in this PR should not break the existing test (because it would also break users). Could you please undo this change?

(I think dropping Python 3.6 support is past due, but that's a separate step.)

rwgk · 2022-11-21T11:07:25Z

include/pybind11/embed.h

@@ -55,6 +55,7 @@
        PYBIND11_TOSTRING(name), PYBIND11_CONCAT(pybind11_init_impl_, name));                     \
    void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_                                \
                                               & variable) // NOLINT(bugprone-macro-parentheses)
+#define PYBIND11_PYCONFIG_SUPPORT_PY_VERSION (0x03080000)


PYBIND11_PYCONFIG_SUPPORT_PY_VERSION_HEX (for clarity)

rwgk · 2022-11-21T11:55:44Z

include/pybind11/embed.h

@@ -261,6 +283,13 @@ class scoped_interpreter {
        initialize_interpreter(init_signal_handlers, argc, argv, add_program_dir_to_path);
    }

+#if PY_VERSION_HEX >= PYBIND11_PYCONFIG_SUPPORT_PY_VERSION
+    explicit scoped_interpreter(const PyConfig &config, bool add_program_dir_to_path = true) {


I think this needs to become:

explicit scoped_interpreter(PyConfig *config, int argc = 0, const char *const *argv = nullptr, bool add_program_dir_to_path = true) {

I'm not sure how this will shake out best.

I'm thinking the scoped_config will not actually be needed.

I'd definitely split out initialize_interpreter_pre_pyconfig() for clarity and safety (unused variables in one branch or another), in the detail namespace.

Then there would be two initialize_interpreter() overloads:

#if PY_VERSION_HEX >= PYBIND11_PYCONFIG_SUPPORT_PY_VERSION_HEX inline void initialize_interpreter(PyConfig *config, int argc = 0, const char *const *argv = nullptr, bool add_program_dir_to_path = true) { ... implementation here ... } #endif inline void initialize_interpreter(bool init_signal_handlers = true, int argc = 0, const char *const *argv = nullptr, bool add_program_dir_to_path = true) { #if PY_VERSION_HEX < PYBIND11_PYCONFIG_SUPPORT_PY_VERSION_HEX detail::initialize_interpreter_pre_pyconfig( init_signal_handlers, argc, argv, add_program_dir_to_path); #else PyConfig config; PyConfig_InitIsolatedConfig(&config); config.isolated = 0; config.use_environment = 1; config.install_signal_handlers = init_signal_handlers ? 1 : 0; initialize_interpreter(&config, argc, argv, add_program_dir_to_path); #endif }

Then the old scoped_interpreter constructor can stay as-is, and the new one will simply be:

explicit scoped_interpreter(PyConfig *config, int argc = 0, const char *const *argv = nullptr, bool add_program_dir_to_path = true) { initialize_interpreter(config, argc, argv, add_program_dir_to_path); }

Note that I'd choose to make this PyConfig *config instead of PyConfig &config, so that it is more obvious that the config is changed in place (PyConfig_Clear()). It seems very unlikely that anyone would want config to be const, although that's only a best guess. When you're done with this PR (CI passes), we should run it by Victor Stinner (not tagging him just now intentionally).

rwgk · 2022-11-22T05:58:17Z

include/pybind11/embed.h

+        detail::precheck_interpreter();
+        detail::initialize_interpreter(config, argc, argv, add_program_dir_to_path);


I think it will be most useful

to move this initialize_interpreter() overload out of the detail namespace,

and the detail::precheck_interpreter(); call to the very top of that overload, just like for the other overload.

The end result will be that the pair of constructors and the pair of initialize_interpreter() overloads each are almost identical, except for the first argument (bool vs PyCconfig).

It's very little extra work, it looks great already.

rwgk · 2022-11-23T16:49:48Z

The force push was for git rebase master.

I added one simple commit, which was much easier than explaining here: b94d6ba

I just looked, our existing test coverage isn't complete, unless I'm overlooking something, add_program_dir_to_path is not exercised at all. Covering this systematically for both pre-PyConfig and PyConfig would be great, but I see that as optional, covering that just for the new PyConfig overload would be sufficient for this PR I think.

This existing test case looks like a good starting point:

pybind11/tests/test_embed/test_interpreter.cpp

Lines 363 to 383 in 9c18a74

    
           TEST_CASE("sys.argv gets initialized properly") { 
        
               py::finalize_interpreter(); 
        
               { 
        
                   py::scoped_interpreter default_scope; 
        
                   auto module = py::module::import("test_interpreter"); 
        
                   auto py_widget = module.attr("DerivedWidget")("The question"); 
        
                   const auto &cpp_widget = py_widget.cast<const Widget &>(); 
        
                   REQUIRE(cpp_widget.argv0().empty()); 
        
               } 
        
               { 
        
                   char *argv[] = {strdup("a.out")}; 
        
                   py::scoped_interpreter argv_scope(true, 1, argv); 
        
                   std::free(argv[0]); 
        
                   auto module = py::module::import("test_interpreter"); 
        
                   auto py_widget = module.attr("DerivedWidget")("The question"); 
        
                   const auto &cpp_widget = py_widget.cast<const Widget &>(); 
        
                   REQUIRE(cpp_widget.argv0() == "a.out"); 
        
               } 
        
               py::initialize_interpreter(); 
        
           }

Could you please try to morph that into an additional test case, with this at the core?

        py::scoped_interpreter argv_scope(&config, 1, argv, true);

I don't know how to tricky it is to assert that add_program_dir_to_path is handled properly. I hope it's within reason, I'd definitely try.

…ialize_interpreter); config_guard

…ace. Move the `PYBIND11_PYCONFIG_SUPPORT_PY_VERSION_HEX` define down to where it is used for the first time, and check if it is defined already, so that it is possible to customize from the compilation command line, just in case there is some unforeseen issue for Python 3.8, 3.9, 3.10.

rwgk · 2022-11-27T14:52:29Z

LGTM but

🐍 3 • ICC latest • x64 (using Python 3.8)
🐍 3 • almalinux:9 • x64 (using Python 3.9)

now produce similar errors. Do you have ideas already?

I think the segfaults could be side-effects of the failed REQUIRE( get_sys_path_size() == sys_path_default_size + 1 ) and are possibly resolved when the REQUIRE is fixed.

rwgk · 2022-11-27T21:46:56Z

Interesting, I see you changed the expecations from == default + 1 to > default.

Did you look why it is not default + 1 in the two failure cases?

arman-novikov · 2022-11-27T21:53:50Z

it seems a value remains in sys path left by the previous call of scoped_interpreter ctor

rwgk · 2022-11-27T22:17:17Z

it seems a value remains in sys path left by the previous call of scoped_interpreter ctor

Not good ... but that sounds like a problem in CPython.

From the previous run it seems to be a problem only for Python 3.8 and 3.9. Maybe only some platforms?

But — I don't want to turn this into a science project.
But also — I want to keep the test as strict as possible.

What do you think about a simple "future-strict" approach? Something like:

#if PY_VERSION_HEX < 0x030A0000
    // It seems a value remains in sys.path left by the previous call of scoped_interpreter ctor.
    REQUIRE(get_sys_path_size() > sys_path_default_size);
#else
    REQUIRE(get_sys_path_size() == sys_path_default_size + 1);
#endif

IIRC the 1st REQUIRE(get_sys_path_size() == sys_path_default_size + 1); succeeded everywhere, only the 2nd failed and would need that #if.

rwgk

I really like the last commit!

@Skylion007 could you please take another look?

rwgk · 2022-11-28T00:44:06Z

I forgot to mention: it would be great if you could update the PR description and changelog entry.

arman-novikov · 2022-11-30T22:02:28Z

@Skylion007 would you please share your review? I've already covered new (and some current) features with tests

Skylion007

Looks good to me.

rwgk · 2022-12-01T05:17:52Z

Thanks @arman-novikov for all the work, thanks @Skylion007 for the review!

arman-novikov marked this pull request as draft November 13, 2022 15:23

arman-novikov marked this pull request as ready for review November 13, 2022 15:44

Skylion007 requested changes Nov 13, 2022

View reviewed changes

rwgk reviewed Nov 16, 2022

View reviewed changes

Skylion007 reviewed Nov 16, 2022

View reviewed changes

arman-novikov requested a review from henryiii as a code owner November 18, 2022 22:24

arman-novikov requested review from Skylion007 and removed request for henryiii November 18, 2022 22:41

rwgk reviewed Nov 21, 2022

View reviewed changes

rwgk reviewed Nov 22, 2022

View reviewed changes

rwgk force-pushed the scoped_interp_from_pyconfig branch from cc89470 to b94d6ba Compare November 23, 2022 16:35

arman-novikov and others added 12 commits November 26, 2022 11:22

scoped_interpreter overloaded ctor: PyConfig param

c77140e

style: pre-commit fixes

8fd4852

refact: some logics extracted into funcs (precheck_interpreter, _init…

28cc566

…ialize_interpreter); config_guard

style: pre-commit fixes

8770abe

refact: scoped_config, some funcs hidden in detail ns

58073f1

refact: macro PYBIND11_PYCONFIG_SUPPORT_PY_VERSION + undef

fc8ba37

feat: PYBIND11_PYCONFIG_SUPPORT_PY_VERSION set to 3.8

80e6404

tests: Custom PyConfig

5eb081a

ci: python 3.6 -> 3.8

5ab1132

ci: reverted py 38 back to 36; refact: initialize_interpreter overloads

4444ea1

style: pre-commit fixes

f1f1ecd

fix: readability-implicit-bool-conversion

e1a486e

arman-novikov and others added 4 commits November 26, 2022 11:22

refact: each initialize_interpreter overloads in pybind11 ns

e25e97b

tests: Add program dir to path, Custom PyConfig with argv

7f22034

refact: clang-formatted

e5e8c28

arman-novikov force-pushed the scoped_interp_from_pyconfig branch from 4794892 to e5e8c28 Compare November 26, 2022 09:22

tests: Add-program-dir-to-path covers both scoped_interpreter overloads

ed662de

tests: Add-program-dir-to-path fixed

f11fcb9

tests: Add-program-dir-to-path py_version dependant validation

6907f5a

rwgk approved these changes Nov 28, 2022

View reviewed changes

Skylion007 approved these changes Dec 1, 2022

View reviewed changes

rwgk merged commit 8869984 into pybind:master Dec 1, 2022

github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 1, 2022

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Dec 1, 2022

Fix test added with PR pybind#4330

699321a

rwgk mentioned this pull request Dec 1, 2022

Fix test added with PR #4330 #4372

Merged

rwgk pushed a commit that referenced this pull request Dec 1, 2022

Fix test added with PR #4330 (#4372)

358ba45

henryiii removed the needs changelog Possibly needs a changelog entry label Dec 20, 2022

rwgk mentioned this pull request Feb 11, 2023

FWD pybind11 google/pybind11clif#4330

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scoped_interpreter. overloaded constructor: PyConfig param #4330

scoped_interpreter. overloaded constructor: PyConfig param #4330

arman-novikov commented Nov 13, 2022 •

edited

Loading

Skylion007 left a comment

Skylion007 Nov 13, 2022 •

edited

Loading

arman-novikov Nov 13, 2022

Skylion007 Nov 13, 2022

rwgk commented Nov 13, 2022 •

edited

Loading

rwgk commented Nov 13, 2022

rwgk left a comment

rwgk Nov 16, 2022

arman-novikov Nov 16, 2022

rwgk Nov 16, 2022

rwgk Nov 16, 2022

Skylion007 Nov 16, 2022

Skylion007 Nov 16, 2022

rwgk commented Nov 16, 2022

arman-novikov commented Nov 18, 2022

arman-novikov commented Nov 18, 2022

rwgk Nov 21, 2022

rwgk Nov 21, 2022

rwgk Nov 21, 2022

rwgk Nov 22, 2022

rwgk commented Nov 23, 2022

rwgk commented Nov 27, 2022

rwgk commented Nov 27, 2022

arman-novikov commented Nov 27, 2022

rwgk commented Nov 27, 2022

rwgk left a comment

rwgk commented Nov 28, 2022

arman-novikov commented Nov 30, 2022

Skylion007 left a comment

rwgk commented Dec 1, 2022

		@@ -88,6 +88,42 @@ inline wchar_t widen_chars(const char safe_arg) {

		PYBIND11_NAMESPACE_END(detail)

		inline void precheck_interpreter() {

		detail::precheck_interpreter();
		detail::initialize_interpreter(config, argc, argv, add_program_dir_to_path);

scoped_interpreter. overloaded constructor: PyConfig param #4330

scoped_interpreter. overloaded constructor: PyConfig param #4330

Conversation

arman-novikov commented Nov 13, 2022 • edited Loading

Description

Suggested changelog entry:

Skylion007 left a comment

Choose a reason for hiding this comment

Skylion007 Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Nov 13, 2022 • edited Loading

rwgk commented Nov 13, 2022

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Nov 16, 2022

arman-novikov commented Nov 18, 2022

arman-novikov commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Nov 23, 2022

rwgk commented Nov 27, 2022

rwgk commented Nov 27, 2022

arman-novikov commented Nov 27, 2022

rwgk commented Nov 27, 2022

rwgk left a comment

Choose a reason for hiding this comment

rwgk commented Nov 28, 2022

arman-novikov commented Nov 30, 2022

Skylion007 left a comment

Choose a reason for hiding this comment

rwgk commented Dec 1, 2022

arman-novikov commented Nov 13, 2022 •

edited

Loading

Skylion007 Nov 13, 2022 •

edited

Loading

rwgk commented Nov 13, 2022 •

edited

Loading