Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add nutils requirements #439

Merged
merged 5 commits into from
Jan 25, 2024
Merged

Add nutils requirements #439

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a413d7f
Remove unnecessary statements. Move time step to a better place.
BenjaminRodenberg Jan 15, 2024
7b34e0a
More rearranging.
BenjaminRodenberg Jan 15, 2024
b0d71a8
Use requirements.txt and create venv in run.sh.
BenjaminRodenberg Jan 17, 2024
711ac90
Revert readability changes. Moved to #443.
BenjaminRodenberg Jan 17, 2024
0796b87
Update perpendicular-flap/solid-nutils/requirements.txt
BenjaminRodenberg Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions channel-transport/fluid-nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils==7
pyprecice==3.0.0.0dev2
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are adding a requirements.txt, I think we should also specify here the non-preCICE dependencies. For this script:

from nutils import function, mesh, cli, solver, export
import treelog as log
import numpy as np
import precice
from mpi4py import MPI

Similarly for the rest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's the state-of-the-art here. Sometimes there are requirement.txt files that just originate from a pip3 freeze. This is extreme and a huge overspecification and leads to the installation of a lot of unnecessary packages. I had this situation a few times and it is just annoying.

Adding treelog, numpy, and mpi4py sounds reasonable, but might also be unnecessary as long as it does not cause problems, if it is missing in the requirements.txt. I imagine numpy is already a dependency of nutils. I also cannot pin down a reasonable version number here and don't know how much value it has, if we do not specify a version (imagine somebody using an ancient numpy).

In the end this is also one more thing we have to maintain and keep in sync with the script (we will forget to remove treelog, if we decide to use a different logger occasionally) and to me the usefulness is not clear as long at things do not break without it. Additionally, it's some work to add the additional lines in every file now (Writing this comment was more work, but still, I'm lazy).

Copy link
Member

@MakisH MakisH Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we would anyway need to tell the user "install treelog, numpy, mpi4py manually" (or hope that they are already installed by something else) and then tell them also "install the rest from the requirements.txt". This sounds non-ideal to me.

In the C++ world, there is a good practice of explicitly including the headers you need where you need them (but not their dependencies). I would assume the same holds for Python.

Regarding the versions, we can also specify >=, if that is the common problem: https://pip.pypa.io/en/stable/reference/requirement-specifiers/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said: I don't think it is necessary. nutils already requires treelog (https://github.com/evalf/nutils/blob/e1a61d2611d1a7507d6f6dbd2ade25ea52001cad/pyproject.toml#L15). pyprecice requires numpy and mpi4py (https://github.com/precice/python-bindings/blob/49c2af0ed3817bb90afbe2c4e4a21a7b4a704bc8/pyproject.toml#L3). So it is specified indirectly. Yes, dependencies of dependencies might change and then we might get a problem here. But the problem this issue is dealing with is that nutils was not specified properly and right now I only see additional maintainment work and not really so much value in adding more dependencies. (We can easily extend this framework we introduced now, if users run into problems and we know more about the concrete situation)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this is the right approach (I still think we should specify direct dependencies explicitly if we specify dependencies somewhere).

This is a detail that currently matters very little, so I don't think it is blocking. Just the right and complete thing to do, without any clear downsides to me (apart from making the venv even larger).

Just for mpi4py, you could argue that we should be enforcing the same MPI version for every involved component. For which none of the two approaches helps.

3 changes: 3 additions & 0 deletions channel-transport/fluid-nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
#!/bin/sh
set -e -u

python3 -m venv .venv
. .venv/bin/activate
Comment on lines +4 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would hesitate to do something like this for our tutorials. In most of the cases, we rely on users (myself included) having system level Nutils installation(s).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much experience with virtual environments, but I think (as @fsimonis suggested) that we do really need virtual environments for Nutils. We currently use different versions for each tutorial, we should better document this in machine-readable format.

So far, we frequently had the issue that we could not easily update all cases to the same version and we are lacking expertise/maintainership to constantly update. That would help and make the tests reproducible.

Besides, having a system-wide or user-wide installation does not conflict: It would pick the dependencies from the venv anyway.

We should probably check/update our documentation regarding our installation instructions/recommendations for Nutils.

pip install -r requirements.txt
python3 fluid.py
2 changes: 2 additions & 0 deletions flow-over-heated-plate/solid-nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils==7
pyprecice==3.0.0.0dev2
4 changes: 3 additions & 1 deletion flow-over-heated-plate/solid-nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#!/bin/bash
set -e -u

python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements.txt
python3 solid.py

2 changes: 2 additions & 0 deletions partitioned-heat-conduction-direct/nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils==7
pyprecice==3.0.0.0dev2
MakisH marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions partitioned-heat-conduction-direct/nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ if [ -z "$*" ] ; then
usage
fi

python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements.txt

while getopts ":dn" opt; do
case ${opt} in
d)
Expand Down
2 changes: 2 additions & 0 deletions partitioned-heat-conduction/nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils==7
pyprecice==3.0.0.0dev2
4 changes: 4 additions & 0 deletions partitioned-heat-conduction/nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ if [ -z "$*" ] ; then
usage
fi

python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements.txt

while getopts ":dn" opt; do
case ${opt} in
d)
Expand Down
2 changes: 2 additions & 0 deletions perpendicular-flap/fluid-nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils==6
pyprecice==3.0.0.0dev2
3 changes: 3 additions & 0 deletions perpendicular-flap/fluid-nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
#!/bin/sh
set -e -u

python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements.txt
python3 fluid.py
2 changes: 2 additions & 0 deletions perpendicular-flap/solid-nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils
BenjaminRodenberg marked this conversation as resolved.
Show resolved Hide resolved
pyprecice==3.0.0.0dev2
3 changes: 3 additions & 0 deletions perpendicular-flap/solid-nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
#!/bin/sh
set -e -u

python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements.txt
python3 solid.py richoutput=no
2 changes: 2 additions & 0 deletions two-scale-heat-conduction/macro-nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils==7
pyprecice==3.0.0.0dev2
3 changes: 3 additions & 0 deletions two-scale-heat-conduction/macro-nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
#!/bin/sh
set -e -u

python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements.txt
NUTILS_RICHOUTPUT=no python3 macro.py
2 changes: 2 additions & 0 deletions two-scale-heat-conduction/micro-nutils/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
nutils==7
pyprecice==3.0.0.0dev2
4 changes: 4 additions & 0 deletions two-scale-heat-conduction/micro-nutils/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ set -e -u

usage() { echo "Usage: cmd [-s] [-p n]" 1>&2; exit 1; }

python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements.txt

# Check if no input argument was provided
if [ -z "$*" ] ; then
echo "No input argument provided. Micro Manager is launched in serial"
Expand Down
Loading