Use PyConfig_InitPythonConfig instead of PyConfig_InitIsolatedConfig

[danielcjacobs]

Description

Attempts to address #4471

initialize_interpreter initializes an isolated configuration via PyConfig_InitIsolatedConfig. The docs on this config say:

This configuration ignores global configuration variables, environment variables, command line arguments (PyConfig.argv is not parsed) and user site directory. The C standard streams (ex: stdout) and the LC_CTYPE locale are left unchanged. Signal handlers are not installed.

After initializing this config, the function then sets config.isolated to false, config.use_environment to true, and config.install_signal_handlers to the parameter passed to initialize_interpreter, which undoes most of the characteristics that make this an isolated config.

The isolated configuration also says it ignores command line arguments, yet we pass them along anyway.

On top of that, this configuration ignores the user site directory, which means the interpreter won't be able to find any modules installed there (matplotlib is an example of a module that usually defaults to the user site directory)

It seems that PyConfig_InitPythonConfig makes much more sense here, as the docs for this configuration say:

Environments variables and command line arguments are used to configure Python, whereas global configuration variables are ignored.

This seems much more consistent with what we actually want.

Note:

I can see this was brought up in the discussion for #4119, specifically here, though I'm a bit confused as to what the reasoning was behind sticking with the isolated config. Seems like maybe it was just that a unit test was failing. If that test still fails with this MR I may have time to play around and figure out why.

UPDATE:

After some digging, it seems that the necessary change that was missing in #4119 is setting config.parse_argv to 0, which is the value used for the isolated configuration. If we just set this to 0, PyConfig_InitPythonConfig seems to work.

Suggested changelog entry:

``py::initialize_interpreter()`` using ``PyConfig_InitPythonConfig()`` instead of ``PyConfig_InitIsolatedConfig()``, to obtain complete ``sys.path`` (resolves #4471).

[rwgk]

Hi @danielcjacobs, are you aware of #4119 already?

Super-over-simplified: there was a lot of back and forth, ultimately I was going by advice from @vstinner

[rwgk]

Lots of failures.

When the CI is finished, there should be a "Download log archive" option under the top-right gear here:

https://github.com/pybind/pybind11/actions/runs/4025749264/jobs/6919506867

I recommend using that to get the zip file, unpack, systematically analyze what's failing. (You only need to inspect *.txt files at the top level. You can ignore subdirs, those only have redundant info AFAIK.)

At looks like mostly >=3.9 Linux & macOS are failing. Windows seems to be fine. I'd spend 10 minutes to be sure exactly what platforms are and are not affected, before continuing experimenting.

[danielcjacobs]

@rwgk is there a simple way to run tests locally?

[rwgk]

@rwgk is there a simple way to run tests locally?

See .github/workflows/ci.yml. They main option is

cmake --target cpptest

To see the commands emitted:

VERBOSE=1 cmake --target cpptest

[rwgk]

Wow ... this works, really? :-)

LGTM

[rwgk]

@vstinner is there a chance that you could look at this tiny change? What do you think?

It boils down to just this:

-    PyConfig_InitIsolatedConfig(&config);
-    config.isolated = 0;
-    config.use_environment = 1;
+    PyConfig_InitPythonConfig(&config);
+    config.parse_argv = 0;

[danielcjacobs]

Wow ... this works, really? :-)

@rwgk @vstinner OH I believe I figured out why.

The docs on parse_argv say:

If equals to 1, parse argv the same way the regular Python parses command line arguments, and strip Python arguments from argv.

This sentence confused me for quite a while, but after playing around, it just seems to mean if the value is 1, it will do what "regular python" does and strip the first argument (which it expects to just be python). If 0, it will keep all the arguments.

I added a unit test in 699aee5 to demonstrate this behavior.

[Skylion007]

This looks good to me. Thoughts @rwgk?

[rwgk]

My suggestions are more-or-less just nits. The core change seems much more intuitive to me, too.

I have two concerns that are more higher-level:

1. Ideally addressed in this PR, if we can find a reasonably practical way to implement: a new unit test exercising the original matplotlib in ~/.local/python3.11/site-packages situation. Is my understanding correct that we still don't have that test, even with this PR as it is right now?

2. Definitely as a separate project: While looking around this doubt grew bigger: are we sure we really have it right now?

In particular:

• How are "regular" Python command line options (https://docs.python.org/3/using/cmdline.html#using-on-cmdline) processed in the two code branches for PYBIND11_PYCONFIG_SUPPORT_PY_VERSION_HEX false/true? Is the behavior actually consistent between the two?

• Highly speculative: could it be that the default parse_argv = 1 is actually better if we make adjustments elsewhere in embed.h? Specifically, could that help us eliminating this very kludgy looking code:

pybind11/include/pybind11/embed.h

Lines 164 to 167 in a500f43

	PyRun_SimpleString("import sys, os.path; "
	"sys.path.insert(0, "
	"os.path.abspath(os.path.dirname(sys.argv[0])) "
	"if sys.argv and os.path.exists(sys.argv[0]) else '')");

[danielcjacobs]

1. Ideally addressed in this PR, if we can find a reasonably practical way to implement: a new unit test exercising the original matplotlib in ~/.local/python3.11/site-packages situation. Is my understanding correct that we still don't have that test, even with this PR as it is right now?

Correct, I haven't added a test for this. To test it, we'd have to install a package to the user site directory, and try to import it in the test. I'm not sure the how to do the first part.

• Highly speculative: could it be that the default parse_argv = 1 is actually better if we make adjustments elsewhere in embed.h? Specifically, could that help us eliminating this very kludgy looking code:

pybind11/include/pybind11/embed.h

Lines 164 to 167 in a500f43

	PyRun_SimpleString("import sys, os.path; "
	"sys.path.insert(0, "
	"os.path.abspath(os.path.dirname(sys.argv[0])) "
	"if sys.argv and os.path.exists(sys.argv[0]) else '')");

I am thinking if we're not running in an isolated environment, then we wouldn't need to append the path of the running file (regardless of what parse_argv is), but I don't really know why this code exists.

I also don't know if it would be valid to set parse_argv to 1, since this simulates calling our python code from the command line. With parse_argv set to 0, we can just pass the necessary program arguments directly to the python code without worrying about any specific Python args.

[Skylion007]

Weird flake on pre-commit CI, local pre-commit work so it looks good.

[rwgk]

Weird flake on pre-commit CI, local pre-commit work so it looks good.

I've never seen that error before. I'll retry the CI via close-reopen here.

The changes look good to me.

[rwgk]

Correct, I haven't added a test for this. To test it, we'd have to install a package to the user site directory, and try to import it in the test. I'm not sure the how to do the first part.

Messing with the environment will be tricky indeed. I'm not happy to let this slip, but I'm ok with it, for practical reasons. We're not making anything worse.

[rwgk]

Weird flake on pre-commit CI,

It's not a flake unfortunately: home-assistant/core#86892

@henryiii what could we do about this?

Especially in light of this comment: home-assistant/core#86892 (comment)

[rwgk]

Let's wait a day or so to see if @henryiii can help us with the pre-commit issue right here, for simplicity. If not, we can merge this PR and open another one to fix the issue separately.

[henryiii]

If it's the isort issue, just bump isort. Or we can move (and combine our other python linters) to Ruff. I thought the error was different.

[rwgk]

If it's the isort issue, just bump isort.

Trying here: #4480

Thanks!