-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update OpenFOAM demo #32
Conversation
The OpenFOAM demo can trigger this issue with Also opened a pr to the docs with the workaround: EESSI/docs#172 |
OpenFOAM/bike_OpenFOAM8.sh
Outdated
surfaceFeatures 2>&1 | tee log.surfaceFeatures | ||
blockMesh 2>&1 | tee log.blockMesh | ||
decomposePar -copyZero 2>&1 | tee log.decomposePar | ||
mpirun -np $NP -ppn $PPN -hostfile hostlist snappyHexMesh -parallel -overwrite 2>&1 | tee log.snappyHexMesh |
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.
A hostfile is a must here without which the mpirun command would fail.
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.
Why is this true here but not below?
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.
Rather than try to support multi-node in the demos, I think it would be better to just add the disclaimer that they are prepared for single node use cases, but could be extended to multi-node cases. If you submit this job via SLURM you wouldn't need the complication of a hostfile
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 this stands, I don't think it would work would it since there is no hostlist
file?
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.
Output file should be removed
OpenFOAM/bike_OpenFOAM8.sh
Outdated
surfaceFeatures 2>&1 | tee log.surfaceFeatures | ||
blockMesh 2>&1 | tee log.blockMesh | ||
decomposePar -copyZero 2>&1 | tee log.decomposePar | ||
mpirun -np $NP -ppn $PPN -hostfile hostlist snappyHexMesh -parallel -overwrite 2>&1 | tee log.snappyHexMesh |
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.
mpirun -np $NP -ppn $PPN -hostfile hostlist snappyHexMesh -parallel -overwrite 2>&1 | tee log.snappyHexMesh | |
mpirun -np $NP --oversubscribe snappyHexMesh -parallel -overwrite 2>&1 | tee log.snappyHexMesh |
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.
Yes, I have removed this in my latest script which @laraPPr is still testing. Her next commit should make things clear.
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.
In Satish script for version 11 it is changed to this mpirun -np $NP -npernode $PPN snappyHexMesh -parallel -overwrite 2>&1 | tee log.snappyHexMesh
. And that one now works correctly. I'm also less inclined to change this one because it still does something and is only their for the OpenFOAM in the pilot repo.
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 really have a problem with this (since it will soon be lost to history anyway), but the file hostlist
does not exist as far as I can see, so it really does nothing (or is it just a workaround for the use of -ppn
?).
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.
Actually -ppn
is deprecated and -npernode
is to be used instead with mpirun
. Indeed the hostlist thing didn't make any sense to me so I also removed it as @ocaisa did. We can also get rid of --oversubscribe
option and just declare that one needs to have at least 8 cores ( I would say we even need more) to run this script.
Can we change |
There's a lot going on in this PR, and there may be some parts in the current demo script that isn't fully correct. Rather than trying to fix all problems in one go, we should try and get the minimal things done so the OpenFOAM demo can be run using the OpenFOAM installations available in This demo is by no means meant to be a perfect example of running OpenFOAM, so in the short term it's fine if some of the substeps aren't currently doing what they should be doing. The main thing is that we can show people that the OpenFOAM installation provided by EESSI runs (not that we're experts in OpenFOAM). So, perhaps we should open a separate PR first with the minimal work to do to get the OpenFOAM demo running with |
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.
Tested with software.eessi.io
and worked just fine
As a side point, on 4 cores I actually got an MPI error from Having said that, I agree with @boegel that we should just get this merged and worry about tweaking the scripts later (or never 😛 ). |
Oh yeah, we do not check the error codes from the previous steps at all. :D Btw what was the error. I am curious. :) |
|
Ok So I now remove the |
Co-authored-by: ocaisa <[email protected]>
@ocaisa is it now ok? Or should I change something else? |
That looks like a segfault due to memory limit? |
Could be, I didn't investigate |
@boegel I updated the PR |
I'm merging this and following up with a PR to drop the use of the pilot |
No description provided.