Include the current working directory in PYTHONPATH

[vyasr]

The commands in the makefile all run via python -m .... Such commands will not properly find the desired modules unless the current directory is on the PYTHONPATH. Furthermore, since these commands all internally launch subshells that run python -m... (via execute_all) the PYTHONPATH must also be propagated to those subshells. For that, we must export.

The old code wiped the PYTHONPATH. If that is still desirable, I can just set it to be the PWD instead of prepending PWD to the existing PYTHONPATH.

Split out of #146

[ritchie46]

Just out of curiosity, when did this trigger a problem? I've never encountered one whilst running python -m ... with the .venv activated.

[vyasr]

I think the issue is likely a bug in either CPython that is due to some subtle interplay with build options. I reported python/cpython#107353 a couple of years ago, and the problem appears to be similar, but in this case occurs even without PYTHONPATH set (hence why I am suspecting build options). If I unset PYTHONPATH to be safe and then use a few different installations of python on different platforms (Linux/Mac, inside/outside containers, conda/pyenv/system) I see different results for

❯ python -c "import sys; print('' in sys.path)"
True # Sometimes this is False

python -m ... works when the empty string (i.e. the current working directory) is part of sys.path, and fails when it is not. I am not sure under what circumstances it is not included. It seems like a bug to me that this would be possible based on the documentation of sys.path so I can dig a bit further if you would like, but since this is happening in the installation of python itself it seems like we might as well make the safe choice in this repository and ensure that the current working directory is explicitly added to the PYTHONPATH.

One possible root cause is that I see different results for sysconfig.get_config_var('PYTHONPATH') being None vs the empty string, so maybe there is some code inside the sys module that leverages that differently that I haven't found yet. I haven't investigated far enough to really know for sure, though.

[wence-]

It might be (@vyasr) that your environment has PYTHONSAFEPATH set to a non-empty value.

[vyasr]

@wence- good call! It didn't occur to me to check that.

It seems like PYTHONSAFEPATH is set to a nonempty value on the systems where I observe this behavior. Unfortunately, I can't seem to get it unset in the context of the Makefile. It seems like I don't understand variable scopes from a Makefile well enough, or when subshells are being lost. For instance, this works fine:

PYTHONSAFEPATH= .venv/bin/python -m queries.polars

if run directly, but it does not work if I change the run-polars target to include that. Nor does export PYTHONSAFEPATH= or unexport PYTHONSAFEPATH at the top of the Makefile. Any idea what the magic incantation is to get this to stick correctly in the context of make commands?

[vyasr]

@ritchie46 how would you like me to proceed here? If I do figure out how to get PYTHONSAFEPATH unset, would you be open to adding that into the benchmark suite? If so, any ideas on what I might be doing wrong above?

[wence-]

if run directly, but it does not work if I change the run-polars target to include that.

Including PYTHONSAFEPATH= $(VENV)/bin/python ... works fine for me. But probably the problem you have is that more than one target needs that environment var to be set.

This patch is sufficient for me (against main) to run when PYTHONSAFEPATH is set in the environment:

diff --git a/Makefile b/Makefile
index 848dc31..d5304ba 100644
--- a/Makefile
+++ b/Makefile
@@ -5,6 +5,7 @@ SHELL=/bin/bash
 VENV=.venv
 VENV_BIN=$(VENV)/bin
 
+export PYTHONSAFEPATH =
 .venv:  ## Set up Python virtual environment and install dependencies
 	python3 -m venv $(VENV)
 	$(MAKE) install-deps

[vyasr]

if run directly, but it does not work if I change the run-polars target to include that.

Including PYTHONSAFEPATH= $(VENV)/bin/python ... works fine for me. But probably the problem you have is that more than one target needs that environment var to be set.

This patch is sufficient for me (against main) to run when PYTHONSAFEPATH is set in the environment:

diff --git a/Makefile b/Makefile
index 848dc31..d5304ba 100644
--- a/Makefile
+++ b/Makefile
@@ -5,6 +5,7 @@ SHELL=/bin/bash
 VENV=.venv
 VENV_BIN=$(VENV)/bin
 
+export PYTHONSAFEPATH =
 .venv:  ## Set up Python virtual environment and install dependencies
 	python3 -m venv $(VENV)
 	$(MAKE) install-deps

I tested that exact diff earlier and it did not work for me. Let me test again just in case I missed something.

[vyasr]

The issue is that PYTHONSAFEPATH is being set in the bashrc files, so any subshell that is launched winds up having this problem. So I think the options are:

1. The original solution in this PR: add the current directory to PYTHONPATH and move on.
2. Unset PYTHONSAFEPATH as part of the execution of every command. This is a bit messier IMO but not entirely unpalatable since we could set a PYTHON=PYTHONSAFEPATH= $(VENV_BIN)/python variable and use that for every Python invocation.
3. The status quo, which is that in any environment with PYTHONSAFEPATH=1 the make commands will not work.

[vyasr]

Any thoughts on how best to proceed with this PR?

[ritchie46]

Sorry for the delay on my part. Seeing the discussion, I agree on

The original solution in this PR: add the current directory to PYTHONPATH and move on.

Can you add a comment on why we do this on the location where we append current directory? (e.g. the PYTHONSAFEPATH discussion above).

After a rebase, I think it is good to merge.