Fix Python bindings version ignored in system tests#745
Fix Python bindings version ignored in system tests#745AdityaGupta716 wants to merge 24 commits into
Conversation
|
This pull request has been mentioned on preCICE Forum on Discourse. There might be relevant details there: https://precice.discourse.group/t/gsoc-2026-aditya-gupta/2773/4 |
…ix --system-site-packages typo in fluid-su2 run.sh
|
@MakisH the component templates were never setting PRECICE_TUTORIALS_NO_VENV=1 (the escape hatch added in #680), and the Docker stages weren't permanently activating /home/precice/venv at runtime. This PR closes both gaps for python_bindings, fenics_adapter, nutils_adapter, and su2_adapter also adds a fast-fail sanity check if precice isn't importable when the escape hatch is set, and fixes a system-site-packages typo in flow-over-heated-plate/fluid-su2/run.sh. Please review! |
…nity check to su2-adapter; fix semnantic typo
MakisH
left a comment
There was a problem hiding this comment.
Looks good and clean, thanks! I did some small modifications, and I still need to test it.
Good catch with the --system-site-package[s] typo!
There was a problem hiding this comment.
Pull request overview
Fixes the issue where the PYTHON_BINDINGS_REF argument was silently ignored by system tests because tutorial run.sh scripts created their own .venv overriding the Docker image's venv. The Docker image now permanently activates the venv via ENV VIRTUAL_ENV/ENV PATH for the Python-based stages, and the system-test component templates set PRECICE_TUTORIALS_NO_VENV=1 so the run scripts skip venv creation. A typo in --system-site-package(s) is also corrected in several tutorial run scripts.
Changes:
- Persist venv activation in
python_bindings,fenics_adapter,nutils_adapter,su2_adapter, andmicro_managerDocker stages. - Set
PRECICE_TUTORIALS_NO_VENV=1and add a precice-import sanity check in four component templates. - Fix
--system-site-package→--system-site-packagesin four tutorialrun.shscripts.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/tests/dockerfiles/ubuntu_2404/Dockerfile | Adds ENV VIRTUAL_ENV / ENV PATH after venv creation in python_bindings, fenics_adapter, nutils_adapter, su2_adapter, and micro_manager stages. |
| tools/tests/component-templates/python-bindings.yaml | Sets PRECICE_TUTORIALS_NO_VENV=1 and adds a precice-import sanity check before running. |
| tools/tests/component-templates/fenics-adapter.yaml | Same as python-bindings template. |
| tools/tests/component-templates/nutils-adapter.yaml | Same as python-bindings template. |
| tools/tests/component-templates/su2-adapter.yaml | Same as python-bindings template, preserving SU2 env vars. |
| flow-over-heated-plate/fluid-su2/run.sh | Fixes --system-site-package typo. |
| elastic-tube-3d/solid-fenics/run.sh | Fixes --system-site-package typo. |
| channel-transport-reaction/fluid-fenics/run.sh | Fixes --system-site-package typo. |
| channel-transport-reaction/chemical-fenics/run.sh | Fixes --system-site-package typo. |
| changelog-entries/745.md | Adds changelog entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
As suggested by Copilot
- Base fenics_adapter, nutils_adapter, micro_manager stages upon python_bindings stage. - Remove the re-creation of the venv, directly activate it. - Remove unused ENV commands.
It seems to be installing in `./venv/` by default.
|
Current state: The FEniCS installation is not using the right venv. I am trying to test this locally, but (1) the FEniCS adapter tests fail due to venv issues, and (2) fieldcompare detects differences in the elastic-tube-1d and Nutils cases. Regarding FEniCS, the error is familiar: When building the container, it looks like a venv is always created at the time of installing The fieldcompare regressions are generally small, but various reasons could be behind them. Latest command I used: I am putting this aside for now, as I am running out of ideas and time. @AdityaGupta716 if you have any suggestions, let me now. |
|
@MakisH Would it make sense to revert fenics_adapter back to FROM precice_dependencies instead of FROM python_bindings, and keep its own --system-site-packages venv as before? That would isolate FEniCS from the global venv change while keeping the fix working for all other Python adapters. |
|
@AdityaGupta716 That would be an option, but it would then lead to an inconsistent state regarding #584. I am trying to understand how the FEniCS adapter Docker image handles that. Resources:
With this system-wide venv, we are essentially reproducing the behavior of Would you like to try if that works, or should I? |
Problem
Tutorial run.sh scripts create their own .venv and install from requirements.txt (e.g. pyprecice~=3.0), silently overwriting the specific PYTHON_BINDINGS_REF version installed by the Docker image. This means the version argument passed to system tests was completely ignored.
Root cause
The Docker image installs the correct version into /home/precice/venv but never permanently activates it. At container runtime the venv isn't on PATH, so the run script sees no active venv and creates its own. PRECICE_TUTORIALS_NO_VENV (added in #680) was never set in the component templates, so the escape hatch was never triggered.
Fix
Add ENV VIRTUAL_ENV and ENV PATH after venv creation in python_bindings, fenics_adapter, and nutils_adapter Dockerfile stages — permanently activating the correct venv at container runtime
Set PRECICE_TUTORIALS_NO_VENV=1 in python-bindings, fenics-adapter, nutils-adapter, and su2-adapter component templates
Add a sanity check that fails fast with a clear error if precice is not importable when the escape hatch is set
Fix --system-site-packages typo in flow-over-heated-plate/fluid-su2/run.sh
Fixes #584