gudhi-devel
gudhi-devel copied to clipboard
GUDHI
Check support for oneTBB
Our code was written for use with TBB, we need to check if it also works with the more recent oneTBB. I think Tangential_complex/benchmark/benchmark_tc.cpp may be problematic. I also reported to Hera their use of parallel_do in https://github.com/anigmetov/hera/issues/7. FindTBB.cmake may be outdated (maybe it should just be removed?). https://software.intel.com/content/www/us/en/develop/documentation/onetbb-documentation/top/onetbb-developer-guide/migrating-from-threading-building-blocks-tbb.html has relevant information. At some point we could investigate using the standard C++17 API to replace most of our uses of TBB, but currently gcc/clang implement that on top of TBB, so it wouldn't remove the TBB dependency.
The mechanism we have with src/cmake/modules/FindTBB.cmake and src/cmake/modules/UseTBB.cmake is broken with latest TBB versions.
If I remove these 2 files, TBB is found, but TBB_LIBRARY_DIRS is no more set (maybe others variables, TBC) which leads to errors in setup.py construction by cmake.
@mglisse Do we want to keep support for the former TBB library, or do we go full with oneTBB ?
CGAL seems to be able to do both, I did not check. They kept the FindTBB.cmake file, which is quite strange to me... I have to retro-engineer this file as we do not have the same.
I think it is too early to drop TBB. For instance, Debian does not have a onetbb package yet (it looks like they are working on it, but it isn't even in experimental yet). Yes, I think CGAL handles both.
With my Debian hat on, I'd like to revive this issue. Debian has now gone through its oneTBB transition, and I've been unable to make GUDHI build with TBB enabled ever since.
I finally have some time on my hands, so if anyone has any pointers to where I should start, I'm happy to put in some work trying to make GUDHI work with oneTBB.
where I should start
The first 2 comments have our best guess as to what is needed. Probably not much to change in the C++ code, just a benchmark, although we won't know until we try. The main work is probably build system stuff. One first issue is how to let cmake detect OneTBB without breaking support for older TBB (several developers have an ubuntu release with an older TBB). It is possible that removing FindTBB.cmake is the right direction (although CGAL seems to manage while keeping it), I think TBBConfig.cmake may have been shipped with TBB since before OneTBB, but then we need to update the way we use it since it doesn't define the same variables as our FindTBB.cmake. The second difficulty is with our mix of CMake and setup.py, we can't just rely on cmake to do its magic, we have to extract the flags it would use so we can pass them to the python stuff, and IIRC that's particularly complicated on windows where cmake may have flags for different build configurations in one variable.
I think it is too early to drop TBB.
At this point, I think it would be better to support only oneTBB, instead of only the old TBB as we do now. Of course both would be best, but not if that delays the support for oneTBB.
I would like to try this approach that looks nice and modern to me. This involves cmake 3.11, but this version is already 4 years old, so it should be ok for everybody. (let's note that compilation is ok on https://github.com/VincentRouvreau/onetbb_cmake_fetch_content it fails because I don't know how to launch an exe in batch :disappointed: )
I think always downloading is problematic: at least Debian will need to disable it, and it may slow down the build significantly, unless I misunderstood how it works (FIND_PACKAGE_ARGS might help with more recent CMake). What is your hope exactly? That it will build only the relevant version of tbb (say Release) and thus the variables will be easier to deal with than when we use find_package on an installed version? Or that you can skip installing?
This is just an opinion, but: Please please try to avoid automatic downloading of code as a way to "solve" dependency problems. It makes life miserable for redistributors (both classical Linux distros and others), and makes anything beyond the most simple "end user" type use needlessly hard. In addition, auto-downloaded (or otherwise bundled) dependencies like these cause incompatibilities with other software that people might want to integrate GUDHI with, and can also end up going stale over time.
Dependency management is hard. Bundling or autodownloading is a poor way to avoid the problem, not a solution to the problem.
(Of course: this is an open source project run by volunteers – it's easy for me to wave my hands around and say "don't do it this way" without putting in the work myself. But it's an opinion I'd still like to make heard.)
Thanks for the feedback. What I like with cmake fetch content feature, is that we can control which onetbb version we are using (direct in the cmake code, and no more in dockerfiles, vcpkg, brew, ...). But the cons of this feature, is that if you remove your build directory, you have to download onetbb again, and if you make clean, you have to build onetbb dynamic libraries again.
Ok, I will try to do it differently. Maintaining the FindTBB.cmake files is also not easy... And there is no clear support from cmake like FindBoost.cmake, for example.
CGAL FindTBB.cmake is not really clear about the copyright for instance... BSD, public domain ? What Copyright.txt file ?
Maintaining the
FindTBB.cmakefiles is also not easy...
Doesn't oneTBB come with TBBConfig.cmake? That should mean that we don't need to bundle a FindTBB.cmake anymore.
(downloading if the dependency is missing may be ok, but it has to be possible to use a preinstalled version)
Marc Glisse @.***> writes:
Doesn't oneTBB come with TBBConfig.cmake? That should mean that we don't need to bundle a FindTBB.cmake anymore.
It does: https://github.com/oneapi-src/oneTBB/tree/e2dca807a1b22dbe2f065b4c50ca0ea7ada7cd70/cmake#tbbconfig---integration-of-binary-packages
I checked, and that TBBConfig.cmake ships at least with Debian, Ubuntu, Fedora and Arch. Probably with all the rest who distribute oneTBB, too.