Fix redirected/"fake" Python executable

[Jongmassey]

We were already redirecting python to be the same version of Python available in the OpenSAFELY python docker image (with corresponding venv/packages), however there were two bugs: this script was not executable and it lacked a bash shebang. The lack of executability didn't seem to bother VS Code, but when called from the CLI it obviously failed. The smoke test now actually calls the python executable rather than just checking it exists, which required the addition of a shebang to the script (which it probably should have had all along).

• add bash shebang to redirected python script (to make smoke test work, amongst others)
• make executable
• make smoke test exercise this python

[iaindillingham]

We redirect python to be the same version of Python available in the OpenSAFELY python docker image (with corresponding venv/packages).

Wasn't this the case previously? I think the shebang and the executable bit have changed, not the redirect.

This fixes a couple of bugs

What bugs? I assume one of them relates to the smoke test. However, the smoke test passes on main; the smoke test passes on Jongmassey/fix-fake-python. For future archaeologists, could you describe the bugs?

and improves the smoke test.

How? Having read and re-read the code, checked out both branches, and built images, I think the smoke test was testing that /usr/bin/python existed, but didn't test that it was a Python. Does the smoke test now test that /usr/bin/python is a Python?

[iaindillingham]

Could you tell me more about what we're accomplishing with printf here? I can see that we're writing two lines to a file, but why printf and not echo?

I appreciate that you didn't last edit this line (@lucyb did 🙂), but could you also help me understand why we're creating a fake system Python and not activating the venv? By not activating the venv, the user doesn't get access to useful tools in /opt/venv/bin, such as ipython and pytest.

[Jongmassey]

a further Google tells me that echo -e supports newline characters, I've always previously reached for printf in that circumstance.

As to why - this stemmed from a chat with Dave and Simon about the possibility of redirecting the system python to one running inside (one of) the OpenSAFELY Python docker container(s), this is scaffolding to make it easier to make that switch in the future. It also lets us point to the venv python before it's activated which fixed another problem I can't quite remember right now.

re: venv activation - the venv is activated by vs code when the codespace/devcontainer starts up. Which venv VSCode chooses to activate is configured by the Python default interpreter setting. This worked fine when we had vs code settings in a vs code settings file in the research template, but became flaky when these settings were moved to the devcontainer.json. By faking the system python and removing the interpreter path setting, vs code reliably activates the venv we want it to at startup

[iaindillingham]

Thanks, @Jongmassey.

To make the intent of this commit clear, could you use echo -e? Diffs are super-informative; the dark green background of -e would be just enough to communicate "We want newlines now, but we didn't before".

It's a shame there isn't a comment about the scaffolding. Could you add one?

I can't see where VS Code activates the venv in .devcontainer/devcontainer.json. Is doing so a feature of ms-python.python?

Whatever should be activating the venv, the venv isn't being activated. We can verify this by typing ipython in the terminal. The result is command not found. I also see that the ehrql imports in the dataset definition are raising syntax errors. Have I misunderstood? If not, I'll create an issue.

[Jongmassey]

https://github.com/opensafely/research-template/blob/improve-devcontainers/.devcontainer/devcontainer.json#L42-L43

           "python.terminal.activateEnvironment": true,
           "python.terminal.activateEnvInCurrentTerminal": true,

[Jongmassey]

OK so activation of the venv in the terminal isn't actually working with this despite me thinking that it did when I tried it locally. I'll poke at it some more until it does.

[iaindillingham]

Thank you 🙂

[iaindillingham]

Actually, @Jongmassey, let's deal with the virtual environment issue separately:

• ehrQL imports display syntax errors in VS Code #19
• The Python venv isn't activated in VS Code's Terminal #20

We can triage these issues next week.

[Jongmassey]

Wasn't this the case previously? I think the shebang and the executable bit have changed, not the redirect.

Yes, sorry I could have used a clearer tense there.

[Jongmassey]

@iaindillingham added wording to clarify what this fixes

[Jongmassey]

I don't think we should be doing this redirection at all, maybe not until we actually need it. What I'd previously diagnosed as the vscode python interpreter setting not working properly in codespaces is in fact the Python extension just taking 30s or so to startup and things like code highlighting and automatic venv activation in terminal not working until it has finished starting up.

[iaindillingham]

Thanks for making the changes, @Jongmassey. I'm going to approve, because I feel that the new form of redirection is better than the old form of redirection: even though the smoke test doesn't tell us whether python is Python, it does tell us that python is executable. The comments also communicate intent, which will be helpful in the future.

[Jongmassey]

Let's not do this for now, this adds unnecessary complexity and doesn't work in all situations