-
-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Add nutils requirements #439
Changes from all commits
a413d7f
7b34e0a
b0d71a8
711ac90
0796b87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
nutils==7 | ||
pyprecice==3.0.0.0dev2 | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
nutils==7 | ||
pyprecice==3.0.0.0dev2 |
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 | ||
|
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
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
nutils==7 | ||
pyprecice==3.0.0.0dev2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
nutils==6 | ||
pyprecice==3.0.0.0dev2 |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
nutils==8 | ||
pyprecice==3.0.0.0dev2 |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
nutils==7 | ||
pyprecice==3.0.0.0dev2 |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
nutils==7 | ||
pyprecice==3.0.0.0dev2 |
There was a problem hiding this comment.
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:Similarly for the rest.
There was a problem hiding this comment.
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 apip3 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
, andmpi4py
sounds reasonable, but might also be unnecessary as long as it does not cause problems, if it is missing in therequirements.txt
. I imaginenumpy
is already a dependency ofnutils
. 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 ancientnumpy
).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).There was a problem hiding this comment.
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 therequirements.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/There was a problem hiding this comment.
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 requirestreelog
(https://github.com/evalf/nutils/blob/e1a61d2611d1a7507d6f6dbd2ade25ea52001cad/pyproject.toml#L15).pyprecice
requiresnumpy
andmpi4py
(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 thatnutils
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)There was a problem hiding this comment.
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.