acts-project
fix: Require TBB to be found by cmake
Previously, the configuration could trun through even if TBB is not present on the system.
📊: Physics performance monitoring for 538aad1a77b3de78e5d0c78a4dd979ee58913bde
physmon summary
- ✅ Particles fatras
- ✅ Particles geant4
- ✅ Particles ttbar
- ✅ Vertices ttbar
- ✅ Truth tracking (KF)
- ✅ Truth tracking (GSF)
- ✅ Truth tracking (GX2F)
- ✅ Truth tracking (KF refit)
- ✅ Truth tracking (GSF refit)
- ✅ CKF finding performance | trackfinding | single muon | truth smeared seeding
- ✅ CKF fitting performance | trackfinding | single muon | truth smeared seeding
- ✅ CKF track summary | trackfinding | single muon | truth smeared seeding
- ✅ Seeding trackfinding | single muon | truth estimated seeding
- ✅ CKF finding performance | trackfinding | single muon | truth estimated seeding
- ✅ CKF fitting performance | trackfinding | single muon | truth estimated seeding
- ✅ CKF track summary | trackfinding | single muon | truth estimated seeding
- ✅ Seeding trackfinding | single muon | default seeding
- ✅ CKF finding performance | trackfinding | single muon | default seeding
- ✅ CKF fitting performance | trackfinding | single muon | default seeding
- ✅ CKF track summary | trackfinding | single muon | default seeding
- ✅ Seeding trackfinding | single muon | orthogonal seeding
- ✅ CKF finding performance | trackfinding | single muon | orthogonal seeding
- ✅ CKF fitting performance | trackfinding | single muon | orthogonal seeding
- ✅ CKF track summary | trackfinding | single muon | orthogonal seeding
- ✅ Seeding trackfinding | 4 muon x 50 vertices | default seeding
- ✅ CKF finding performance | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ CKF fitting performance | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ CKF track summary | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ Ambisolver finding performance | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ IVF notime | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ AMVF gauss notime | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ AMVF grid time | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ Seeding trackfinding | ttbar with 200 pileup | default seeding
- ✅ CKF finding performance | trackfinding | ttbar with 200 pileup | default seeding
- ✅ CKF fitting performance | trackfinding | ttbar with 200 pileup | default seeding
- ✅ CKF track summary | trackfinding | ttbar with 200 pileup | default seeding
- ✅ Ambisolver finding performance | trackfinding | ttbar with 200 pileup | default seeding
- ✅ AMVF gauss notime | trackfinding | ttbar with 200 pileup | default seeding
- ✅ AMVF grid time | trackfinding | ttbar with 200 pileup | default seeding
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Just to put Paul's point another way... why do we need to requite TBB, when ACTS works fine (single-threaded) without it? (Or at least it used to, last time I checked.)
Okay, sure, but compilation fails in that case I think (at least that was what I experienced). Do we test the case where no TBB is present?
Shouldn't we use some more explicit logic here like having an option such as ACTS_EXAMPLES_NO_TBB or something like that?
@benjaminhuth We do have ACTS_USE_EXAMPLES_TBB, we just fall back to OFF if we don't find TBB. I'm ok to make this explicit only without fallback.
I agree, if ACTS_USE_EXAMPLES_TBB is on we should definitely use it without a fallback. Not finding it is an error case IMO
But mostly ACTS_USE_EXAMPLES_TBB is on by default, rather than requested by the user with an explicit -DACTS_USE_EXAMPLES_TBB=ON. In this case, it is nice to have the fallback to OFF to make building ACTS easier for users that just want get something working without having to install tbb or find another CMake option. Is this possible to fall back to OFF iif it isn't specified?
The question remains why it didn't compile for you. That may be a more important issue to solve.
I think you're right @timadye: I should try to reproduce the compile failure.
Anyhow: I think if ACTS_USE_EXAMPLES_TBB=ON and TBB is not found, we should fail the cmake configuration. maybe with a message that indicates that -D ACTS_USE_EXAMPLES_TBB=ON is the right option to make it work.
Maybe I am missing something but this is what is actually change in the PR, right? expect for a more descriptive message
@andiwand Almost, but it's using the TBB_FOUND variable in a way that doesn't make sense anymore without the fallback. I think it's maybe a 10 line refactor to clean it up.
I found the source of the compile errore: The track finding algorithm unconditionally uses tbb::combinable because of the memory statistics. Should we hide this behind a preprocessor define @paulgessinger ?
https://github.com/acts-project/acts/blob/a28acc306ccd11f064c81a68e8eb7f3cc7526c2c/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp#L44
@benjaminhuth True, that was me, and I forgot about it to be honest. I don't have a strong opinion what the best solution is. I would probably lean towards avoiding making the single threaded use case require workarounds in many places, and therefore switch back to hard-requiring TBB?
I think it is a pity to add a required dependency without good reason. It seems this one should be quite easy to #ifndef ACTS_EXAMPLES_NO_TBB around the #include and m_memoryStatistics definition and use (2 places). I think that just means we don't get memory stats in the case without tbb.
A more complete solution would be to make a dummy class like tbbWrap.hpp does, but I don't think that's necessary in this case.
@timadye It's a required dependency for our example framework, not the core lib, but I see your point.
I don't know, your call @benjaminhuth
Okay, during testing I found an additional place with explicit TBB dependency:
https://github.com/acts-project/acts/blob/7c272316b739a72bf45d92e5eb336295e5b41201/Examples/Framework/include/ActsExamples/Framework/Sequencer.hpp#L174
Actually since this involves the FPE mechanism I would now lean towards dropping the possibility to build without TBB...
On the other hand, if we would stick to TBB being optional, we should definitively test it in the CI.
I think that these build errors were not observered for such a long time also indicates the option is not widely used.
OK, I guess I can't argue with that.
Do these direct uses of tbb conflict with tbbWrap's other function of bypassing tbb setup when running in a single-threaded mode? That definitely was useful to simplify debugging.
Do these direct uses of
tbbconflict withtbbWrap's other function of bypassingtbbsetup when running in a single-threaded mode? That definitely was useful to simplify debugging.
I think this should stay untouched, since I only remove the macro-logic in case tbb is not present, but not the wrapper objects.
Quality Gate failed
Failed conditions
25.21% Line Coverage on New Code (required ≥ 50%)
See analysis details on SonarCloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
@benjaminhuth getting_started.md needs an update I believe. I can override the coverage failure afterwards.
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code
See analysis details on SonarCloud
