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

added missing space in 'configopts' in ParaView 5.8.0 easyconfigs using 2020a toolchain #10989

Merged
merged 4 commits into from
Jan 8, 2021

Conversation

mboisson
Copy link
Contributor

There's a missing space at the end of one line of configopts, which means that CMake options get ignored.

@mboisson
Copy link
Contributor Author

mboisson commented Jul 17, 2020

@boegel, you missed something there ;)

The current recipe results in -DOPENGL_INCLUDE_DIR=$EBROOTMESA/include-DVTK_OPENGL_HAS_OSMESA=ON in the cmake options.

@boegel
Copy link
Member

boegel commented Jul 19, 2020

@mboisson Ouch...

Any idea if we can catch this by enhancing the sanity check?

@boegel boegel added the bug fix label Jul 19, 2020
@boegel boegel added this to the next release (4.2.3?) milestone Jul 19, 2020
@boegel boegel changed the title added missing space added missing space in 'configopts' in ParaView 5.8.0 easyconfigs using 2020a toolchain Jul 19, 2020
@boegel
Copy link
Member

boegel commented Jul 19, 2020

Test report by @boegel
FAILED
Build succeeded for 1 out of 2 (2 easyconfigs in this PR)
generoso-x-5 - Linux centos linux 8.2.2004, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/536d329c13f65909153520762d4881a8 for a full test report.

@smoors
Copy link
Contributor

smoors commented Jul 20, 2020

Test report by @smoors
FAILED
Build succeeded for 3 out of 4 (2 easyconfigs in this PR)
node381.hydra.os - Linux centos linux 7.7.1908, x86_64, Intel(R) Xeon(R) Gold 6148 CPU @ 2.40GHz (skylake_avx512), Python 2.7.5
See https://gist.github.com/7b760e6fd4c597de9b5a98c990224900 for a full test report.

@mboisson
Copy link
Contributor Author

Mmmm, so, are those failing tests just a symptoms of the corrected recipe not working, and the incorrect recipe skipping something ?

@smoors
Copy link
Contributor

smoors commented Jul 20, 2020

this is the error for the intel version (foss builds fine). looks indeed like an issue with OSMesa linking, which was skipped previously.

[ 65%] Building CXX object CMakeFiles/vtkFiltersPointsCS.dir/CMakeFiles/vtkFiltersPointsCS/vtkLinearKernelClientServer.cxx.o
/apps/brussel/CO7/skylake/software/impi/2019.7.217-iccifort-2020.1.217/intel64/bin/mpiicpc   -I/tmp/3254107.master01.hydra.brussel.vsc/tmp/vsc10009/build/ParaView/5.8.0/intel-2020a-Python-3.8.2-mpi/easybui
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaDestroyContext'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaMakeCurrent'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaGetCurrentContext'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaCreateContext'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaGetProcAddress'
make[2]: *** [bin/vtkProbeOpenGLVersion-pv5.8] Error 1
make[2]: Leaving directory `/tmp/3254107.master01.hydra.brussel.vsc/tmp/vsc10009/build/ParaView/5.8.0/intel-2020a-Python-3.8.2-mpi/easybuild_obj'
make[1]: *** [VTK/Rendering/OpenGL2/CMakeFiles/vtkProbeOpenGLVersion.dir/all] Error 2

@boegel
Copy link
Member

boegel commented Jul 21, 2020

@boegel one solution would be to allow buildopts and friends be defined a list of strings, which get joined by EB with a space

buildopts can already be a list of string values, but that's used for indicating that the configure-build-test-install cycle myst be run multiple times during a single installation...

@smoors
Copy link
Contributor

smoors commented Jul 21, 2020

@boegel one solution would be to allow buildopts and friends be defined a list of strings, which get joined by EB with a space

buildopts can already be a list of string values, but that's used for indicating that the configure-build-test-install cycle myst be run multiple times during a single installation...

ah yes, I knew that..

@mboisson
Copy link
Contributor Author

this is the error for the intel version (foss builds fine). looks indeed like an issue with OSMesa linking, which was skipped previously.

[ 65%] Building CXX object CMakeFiles/vtkFiltersPointsCS.dir/CMakeFiles/vtkFiltersPointsCS/vtkLinearKernelClientServer.cxx.o
/apps/brussel/CO7/skylake/software/impi/2019.7.217-iccifort-2020.1.217/intel64/bin/mpiicpc   -I/tmp/3254107.master01.hydra.brussel.vsc/tmp/vsc10009/build/ParaView/5.8.0/intel-2020a-Python-3.8.2-mpi/easybui
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaDestroyContext'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaMakeCurrent'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaGetCurrentContext'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaCreateContext'
ld: ../../../lib64/libvtkRenderingOpenGL2-pv5.8.so.5.8: undefined reference to `OSMesaGetProcAddress'
make[2]: *** [bin/vtkProbeOpenGLVersion-pv5.8] Error 1
make[2]: Leaving directory `/tmp/3254107.master01.hydra.brussel.vsc/tmp/vsc10009/build/ParaView/5.8.0/intel-2020a-Python-3.8.2-mpi/easybuild_obj'
make[1]: *** [VTK/Rendering/OpenGL2/CMakeFiles/vtkProbeOpenGLVersion.dir/all] Error 2

actually, I get this error when building with GCC as well, and even if I use Gentoo's libOSMesa.so.
Looking at the intermediate file, it looks like it does not link at all on libOSMesa.so.

Looks like a ParaView bug to me.

@mboisson
Copy link
Contributor Author

mboisson commented Jul 30, 2020

I created an issue on kitware's gitlab... lets see what they say :
https://gitlab.kitware.com/paraview/paraview/-/issues/20120

@Micket
Copy link
Contributor

Micket commented Aug 13, 2020

I'm here to complicate the situation a bit more: #10977

So, I really really really want a version that works with VirtualGL, which will not work if linked with OSMesa.
I would also like this to be the version without any extra suffixes or such so that it's the primary dependency for OpenFOAM

Then, introduce secondary packages for

  • ParaView-5.8.0-foss-2020a-Python-3.8.2-mpi-egl.eb
  • ParaView-5.8.0-foss-2020a-Python-3.8.2-mpi-osmesa.eb

respectively for all your off-screen rendering needs. Probably these versions could just build the pvserver components since this is their usecase.

Since the ParaView we released in the last EB didn't actually build with OSMesa correctly, perhaps we can consider removing it from this PR, and then opening new PRs for a osmesa and egl versions.

@mboisson
Copy link
Contributor Author

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

I have no strong opinion on any of the other differences, like OSPray and whatnot. But, I would suggest we at least do this.

@boegel
Copy link
Member

boegel commented Aug 17, 2020

I'm happy with dropping OSMESA=ON from this Paraview easyconfig, but the implications are not 100% clear to me...

What does enabling OSMESA imply exactly (other than requiring Mesa)?

@Micket
Copy link
Contributor

Micket commented Aug 17, 2020

There are basically 3 modes, all, sort-of-mutually-exclusive, rendering backends:

  1. Onscreen, classic GLX, like your classic glxgears (the X in GLX comes from X11)
  2. Offscreen option 1: OSMesa (off-screen-mesa). Not compatible with VirtualGL last i tried (and I can't imagine how it ever would be). Software rendering only. This is a special non-glx part of Mesa.
  3. Offscreen option 2: EGL (the glorious future). Supports hardware rendering off-screen. This is what you really want for pvserver (if you have GPUs). Mesa also supports EGL with software rendering, so, technically, this could superseed everything in the future. But, VirtualGL doesn't support EGL yet, only GLX. So, for Onscreen needs (via VNC, Xpra, ssh, with VirtualGL) you need GLX mode. (b.t.w EGL is what Wayland uses in case someone is curious)

(all 3 of them make use of Mesa)

@boegel
Copy link
Member

boegel commented Aug 17, 2020

@Micket So that means picking between a Paraview that supports on screen rendering properly (with VirtualGL, no OSMESA) but no offscreen rendering, and one that supports offscreen rendering but no (proper) onscreen rendering?

Don't you basically need both onscreen and offscreen rendering when using Paraview as a dep for OpenFOAM?

Should the Paraview that has good support for on screen rendering be tagged with -gui?

@Micket
Copy link
Contributor

Micket commented Aug 17, 2020

@boegel Yes, it means picking. See ComputeCanada's easyconfigs above in @mboisson comments, where they offer all 3 flavours.

OpenFOAM build their own "parafoam" (instead of just writing a custom file format/file-reader plugin, they compile their own paraview 🤡 ) for post-processing.
OpenFOAM definitely isn't doing off-screen rendering during simulations if that was what you where worrying about, it's all post-processing, just like "normal" paraview usage, but with their hacked version to read their input format.
I have always removed the OSMesa stuff in all our ParaView builds locally because VirtualGL support is crucial
ComputeCanada also uses the onscreen flavour as their dep for OpenFOAM.

Some of the versions need to be tagged, yes. My suggestion above was to rather tag the off-screen versions, like ComputeCanada does, though I'm not sure whether it's good to label it "offscreen-gpu" because, technically, EGL supports software rendering as well (though currently one needs to specify extra environment varialbes to coax it into doing so at runtime #10977 (comment) )

But, I don't really care what versionsuffixes. EGL support for VirtualGL is getting closer and closer, and with some simple patches to VTK, it should definitely be possible for EGL to superseed everything; onscreen and offscreen, gpu and software rendering, so, eventually we could just have 1 golden version again.

@bartoldeman
Copy link
Contributor

In general we instruct direct users of paraview to install its client on their own laptop/workstation/desktop/whatever and then use the server, on the cluster which then will either use a GPU or software rendering, see https://docs.computecanada.ca/wiki/ParaView

For the rest I agree with @Micket here. For us running paraview via virtualgl is a secondary use case but that's just a local detail.

@boegel
Copy link
Member

boegel commented Jan 8, 2021

@Micket @mboisson I would like to get this merged ASAP...

What are the next steps here? The Mesa dependency needs to stay, right? So it's only a matter of test reports & merging?

@Micket
Copy link
Contributor

Micket commented Jan 8, 2021

No matter what we do, we need to have Mesa as a dependency. Mesa supplies the libraries for all 3 possible backends (good old GLX, the obscure "OSMESA" and the new-fangled EGL).

I suggest we

  1. Drop OSMESA from this package, and consider this the on-screen version.
  2. Introduce a new easyconfig with OSMESA support and add a suffix -offscreen
  3. Introduce yet another easyconfig with EGL support and add a suffix -offscreen-gpu

(I have no particular strong feelings about what the suffix should be, just something along these lines)

@mboisson
Copy link
Contributor Author

mboisson commented Jan 8, 2021

No matter what we do, we need to have Mesa as a dependency. Mesa supplies the libraries for all 3 possible backends (good old GLX, the obscure "OSMESA" and the new-fangled EGL).

I suggest we

  1. Drop OSMESA from this package, and consider this the on-screen version.
  2. Introduce a new easyconfig with OSMESA support and add a suffix -offscreen
  3. Introduce yet another easyconfig with EGL support and add a suffix -offscreen-gpu

(I have no particular strong feelings about what the suffix should be, just something along these lines)

These would fit exactly what we do in Compute Canada, so I'm fine with those. For information, these are what we have installed:
https://github.com/ComputeCanada/easybuild-easyconfigs/blob/computecanada-master/easybuild/easyconfigs/p/ParaView/ParaView-5.8.0-gompi-2020a.eb
https://github.com/ComputeCanada/easybuild-easyconfigs/blob/computecanada-master/easybuild/easyconfigs/p/ParaView/ParaView-5.8.0-gompi-2020a-offscreen.eb
https://github.com/ComputeCanada/easybuild-easyconfigs/blob/computecanada-master/easybuild/easyconfigs/p/ParaView/ParaView-5.8.0-gompi-2020a-offscreen-gpu.eb

@Micket
Copy link
Contributor

Micket commented Jan 8, 2021

If there aren't any objections, then I suggest we do this suggested change (which is what we were implicitly already doing here due to the typo) #10989 (review) ?

@boegel
Copy link
Member

boegel commented Jan 8, 2021

If there aren't any objections, then I suggest we do this suggested change (which is what we were implicitly already doing here due to the typo) #10989 (review) ?

Agreed.

@mboisson Can you also remove the -DVTK_OPENGL_HAS_OSMESA=ON?

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel
Copy link
Member

boegel commented Jan 8, 2021

@boegelbot please test @ generoso

@boegelbot
Copy link
Collaborator

@boegel: Request for testing this PR well received on generoso

PR test command 'EB_PR=10989 EB_ARGS= /apps/slurm/default/bin/sbatch --job-name test_PR_10989 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 12398

Test results coming soon (I hope)...

- notification for comment with ID 756862528 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

lgtm

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in total)
generoso-x-3 - Linux centos linux 8.2.2004, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/b8e9d1c7a947f992d9f45040f15d67e8 for a full test report.

@boegel
Copy link
Member

boegel commented Jan 8, 2021

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3504.doduo.os - Linux RHEL 8.2, x86_64, AMD EPYC 7552 48-Core Processor (zen2), Python 3.6.8
See https://gist.github.com/3dfb09d8d8a86ff91f3fa6994e194ce0 for a full test report.

@boegel
Copy link
Member

boegel commented Jan 8, 2021

Going in, thanks @mboisson!

@boegel boegel merged commit 8f6ba5f into easybuilders:develop Jan 8, 2021
@boegel
Copy link
Member

boegel commented Jan 9, 2021

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in total)
node2659.swalot.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz (haswell), Python 3.6.8
See https://gist.github.com/bb70488812dc41a8f4fa036b39b57ff7 for a full test report.

@mboisson mboisson deleted the fix_paraview branch March 26, 2021 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants