fix: Require TBB to be found by cmake

[benjaminhuth]

Previously, the configuration could trun through even if TBB is not present on the system.

[github-actions]

📊: Physics performance monitoring for 538aad1

Full contents

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

[paulgessinger]

The reason for this was that the Sequencer currently falls back to running single threaded in this mode, which was added intentionally. If tbb becomes a hard requirement here, this configuration should be removed as well.

[timadye]

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.)

[benjaminhuth]

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?

[paulgessinger]

@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.

[andiwand]

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

[timadye]

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.

[benjaminhuth]

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.

[andiwand]

Maybe I am missing something but this is what is actually change in the PR, right? expect for a more descriptive message

[paulgessinger]

@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.

[benjaminhuth]

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 ?

acts/Examples/Algorithms/TrackFinding/include/ActsExamples/TrackFinding/TrackFindingAlgorithm.hpp

Line 44 in a28acc3

#include <tbb/combinable.h>

[paulgessinger]

@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?

[timadye]

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.

[paulgessinger]

@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

[benjaminhuth]

Okay, during testing I found an additional place with explicit TBB dependency:

acts/Examples/Framework/include/ActsExamples/Framework/Sequencer.hpp

Line 174 in 7c27231

tbb::enumerable_thread_specific<Acts::FpeMonitor::Result> fpeResult{};

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.

[benjaminhuth]

I think that these build errors were not observered for such a long time also indicates the option is not widely used.

[timadye]

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.

[sonarqubecloud]

Quality Gate failed

Failed conditions
25.21% Line Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

[paulgessinger]

@benjaminhuth getting_started.md needs an update I believe. I can override the coverage failure afterwards.

Invalidated by push of 56d2c1d

[paulgessinger]

I'm ok with this.

[timadye]

lgtm

[sonarqubecloud]

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

[acts-project-service]

✅ Athena integration test results

✅ All tests successful

status	job	report
🟢	report_pull_request
🟢	run_art_test: test_run4_acts_vertex_PU200
🟢	run_art_test: test_run4_ttbar_PU200
🟢	run_art_test: test_data18_13TeV_1000evt
🟢	run_art_test: test_ttbarPU40_reco
🟢	run_workflow_tests_run4_mc
🟢	run_workflow_tests_run2_mc
🟢	run_workflow_tests_run2_data
🟢	run_workflow_tests_run3_mc
🟢	run_workflow_tests_run3_data
🟢	test_ActsDumpGeometryIdentifiers
🟢	test_ActsCheckObjectCountsCachedWorkflow
🟢	test_ActsCheckObjectCountsWorkflow
🟢	test_ActsEFTrackFit
🟢	test_ActsPersistifySeeds
🟢	test_ActsBenchmarkWithSpot
🟢	test_ActsAnalogueClustering
🟢	test_ActsWorkflowHeavyIons
🟢	test_ActsWorkflowFastTracking
🟢	test_ActsWorkflowCached
🟢	test_ActsWorkflow
🟢	test_ActsValidateAmbiguityResolution
🟢	test_ActsValidateResolvedTracks
🟢	test_ActsValidateTracks
🟢	test_ActsValidateActsCoreSpacePoints
🟢	test_ActsValidateActsSpacePoints
🟢	test_ActsValidateSeeds
🟢	test_ActsValidateOrthogonalSeeds
🟢	test_ActsValidateClusters
🟢	test_ActsPersistifyEDM
🟢	test_ActsGSFRefitting
🟢	test_ActsKfRefitting
🟢	test_ActsExtrapolationAlgTest
🟢	test_ActsITkTest
🟢	run_unit_tests