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.

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

Merged
merged 5 commits into from
Jan 25, 2024

Conversation

BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Jan 15, 2024

Stumbled across some minor code readability issues when trying to get the nutils case working. Currently, I'm a bit clueless. @MakisH does this version work on your system? (outdated. Reverted some changes via 711ac90. Moved this part into #443)

  • Add requirements files for clarification w.r.t required version of nutils and pyprecice
  • Use venv in run scripts
  • Motivation: See comment by @fsimonis below.

@uekerman
Copy link
Member

Into which problem do you actually run? Which Nutils version did you use,?

@BenjaminRodenberg BenjaminRodenberg changed the title Fix order statements concerning time step size; possible bug? Fix order statements concerning time step size; add requirements Jan 17, 2024
@BenjaminRodenberg BenjaminRodenberg changed the title Fix order statements concerning time step size; add requirements Add nutils requirements Jan 17, 2024
@BenjaminRodenberg
Copy link
Member Author

Added requirements files together with @fsimonis to pin down the versions.

@MakisH: Can you review and merge? heat-heat is running for me now. The rest I did not test, but should be fine. If the amount of code complication looks like a problem, please try to directly come up with a solution. You probably know much better than I do, because I guess there is something similar in the run.sh files of OpenFOAM.

@IshaanDesai Can you check the macro-micro tutorial w.r.t to the versions?

Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

Do we really want to install Nutils within an env in the run script. What if I have my Nutils already and want to use this one? This is also what we are doing in the VM.
We don't do this for other software, do we?

perpendicular-flap/solid-nutils/requirements.txt Outdated Show resolved Hide resolved
Comment on lines +4 to +5
python3 -m venv .venv
. .venv/bin/activate
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.

Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

It is not clear to me what exactly is this pull request solving/enhancing.

@fsimonis
Copy link
Member

Into which problem do you actually run?

Some tutorials run, some don't. There are no error messages to warn about version conflicts. You need to know about the nutils-adpater page on the website to find out which version to use, and then you need to reinstall nutils if you use a single venv or user pip.

Do we really want to install Nutils within an env in the run script. What if I have my Nutils already and want to use this one? This is also what we are doing in the VM.
We don't do this for other software, do we?

According to the docs, you need multiple versions of nultils to run the tutorials and the latest version isn't supported at all.
The VM installs nutils==7.0, which locks you out of the perpendicular flap.
Multiple required versions means you cannot easily run these things.

It is not clear to me what exactly is this pull request solving/enhancing.

You don't have to manually handle versions of nutils, and you don't have to look up on a website which version you need to run which tutorial.

Python has a solution to the version chaos, being requirements.txt and venv.
If the required nutils versions were consistent, then we could have one requirements file for the tutorials.

An alternative to all of this would be to port the nutils tutorials to work with the latest nutils version. Always assuming the latest nutils version is a sane assumption, given that nutils is distributed via PyPi and not via system package managers.

Co-authored-by: Benjamin Uekermann <benjamin.uekermann@ipvs.uni-stuttgart.de>
@uekerman
Copy link
Member

An alternative to all of this would be to port the nutils tutorials to work with the latest nutils version.

This is always the goal, yes, but we will probably never reach it as new Nutils version come out and the number of tutorials grows.

@fsimonis I see your points, but I am still worried that doing sth like this in the run script is an unexpected side effect.

My suggestion would be:

  • Taking the requirements files. This is good documentation. Users and we can use it if we need it. We could mention them even in the READMEs.
  • Not doing the automatic installation in the run scripts.

@fsimonis
Copy link
Member

I just realized that we already have a port of the perp-flap fluid-nutils waiting to be merged #429

After that, we should be able to use nutils 7 for everything, correct?

In that case, we could add a central requirement file for the tutorials (which also installs tools for plotting etc).
Then in the run.sh scripts, we could source a venv at the root of the tutorials if it exists.

# activate venv if available
[[ -f ../../venv/bin/activate ]] && . ../../venv/bin/activate

# Run participant as usual

@IshaanDesai
Copy link
Member

IshaanDesai commented Jan 18, 2024

My suggestion would be:

* Taking the requirements files. This is good documentation. Users and we can use it if we need it. We could mention them even in the READMEs.

* Not doing the automatic installation in the run scripts.

I second these suggestions. I vote for not doing installing a solver as part of running a tutorial, because this is not what we intend to showcase in a tutorial. With a requirements.txt, a Nutils user would be able to figure out what to do.

Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

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

This is very useful, and I think we should also port it to the training course code. 👍 See comments.

The nutils versions in the requirements agree with the documentation. I only checked the partitioned-heat-conduction.

Feel free to merge after extending the requirements (see comments).

Comment on lines +1 to +2
nutils==7
pyprecice==3.0.0.0dev2
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

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.

Comment on lines +4 to +5
python3 -m venv .venv
. .venv/bin/activate
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.

@MakisH
Copy link
Member

MakisH commented Jan 18, 2024

I just realized that we already have a port of the perp-flap fluid-nutils waiting to be merged #429

After that, we should be able to use nutils 7 for everything, correct?

In that case, we could add a central requirement file for the tutorials (which also installs tools for plotting etc). Then in the run.sh scripts, we could source a venv at the root of the tutorials if it exists.

# activate venv if available
[[ -f ../../venv/bin/activate ]] && . ../../venv/bin/activate

# Run participant as usual

@fsimonis I am not optimistic that we will manage to maintain every nutils tutorial to the same version over the next years. I think it is more robust to define a venv for each tutorial, or at least a requirements.txt for each tutorial. In the partitioned-heat-conduction, with nutils and pyprecice, I get a 100MB .venv. It is not as large as a typical node_modules, but sure, we could reuse some resources.

Would a central venv allow us to specify a different nutils version for each tutorial?

@MakisH
Copy link
Member

MakisH commented Jan 18, 2024

I second these suggestions. I vote for not doing installing a solver as part of running a tutorial, because this is not what we intend to showcase in a tutorial.

We could introduce a setup.sh for such cases.

Note that we are currently inconsistent regarding this:

  • We do build solvers as part of run.sh in the elastic-tube-1d (C++)
  • We don't do that in the OpenFOAM case of partitioned-heat-conduction or the C++ solver of Quickstart.

With a requirements.txt, a Nutils user would be able to figure out what to do.

In the tutorials, I would generally not assume that we have users experienced with the one or the other solver. We often have cases that the user just wants to run a solver to compare/couple to their own.

Generally, the venv system is not very intuitive for people not in the Python world. This needs documentation.

@IshaanDesai
Copy link
Member

* We do build solvers as part of `run.sh` in the `elastic-tube-1d` (C++)

This is a good point. If we already set a precedence of not just running but also building solvers in run.sh, then we can do the same here.

In the tutorials, I would generally not assume that we have users experienced with the one or the other solver. We often have cases that the user just wants to run a solver to compare/couple to their own.

Generally, the venv system is not very intuitive for people not in the Python world. This needs documentation.

+1 vote for this. venv can be confusing as suddenly the terminal switches, and a new user would get confused.

@fsimonis
Copy link
Member

fsimonis commented Jan 21, 2024

Note that we are currently inconsistent regarding this:

We don't do that in the OpenFOAM case of partitioned-heat-conduction or the C++ solver of Quickstart.

This is an open issue/todo. Compilation shouldn't be a hurdle for new users.

I am not optimistic that we will manage to maintain every nutils tutorial to the same version over the next years.

Then this is a strong argument for specifying requirements in the files.

An alternative to the requirements file could also be setup.py which installs an application. Then we could use pipx to install the solver as an application and run it.

Possibly overkill, but could be easier to use.

@MakisH
Copy link
Member

MakisH commented Jan 21, 2024

An alternative to the requirements file could also be setup.py which installs an application. Then we could use pipx to install the solver as an application and run it.

Possibly overkill, but could be easier to use.

I would avoid encouraging users to install anything that may change frequently and that can at the same time be executed without installation. In the past, I observed many users getting confused because they forgot they had an older preCICE version installed somewhere on their system.

@BenjaminRodenberg BenjaminRodenberg merged commit 8ea8a22 into develop Jan 25, 2024
3 checks passed
@BenjaminRodenberg BenjaminRodenberg deleted the fix_nutils_partitioned_heat_conduction branch January 25, 2024 21:23
@MakisH MakisH mentioned this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants