Remove MuonDetectorManager calls from MuonAlignmentCondAlg (ATLASRECTS-5533)

Hi,

this MR removes the non-thread-safe MuonDetectorManager calls from MuonAlignmentCondAlg as described in ATLASRECTS-5533.

Best, Nico

This merge request affects 1 package:

• MuonSpectrometer/MuonConditions/MuonCondGeneral/MuonCondAlg

Adding @rosati ,@nkoehler ,@wleight as watchers

added MuonSpectrometer master review-pending-level-1 labels

CI Result FAILURE (hash d618956a)

	Athena	AthSimulation	AnalysisBase	AthGeneration
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 15468]

Jenkins please retry a build

This merge request affects 1 package:

• MuonSpectrometer/MuonConditions/MuonCondGeneral/MuonCondAlg

Adding @rosati ,@nkoehler ,@wleight as watchers

CI Result FAILURE (hash d618956a)

	Athena	AthSimulation	AnalysisBase	AthGeneration
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 15471]

added review-approved label and removed review-pending-level-1 label

added review-pending-level-1 label and removed review-approved label

Changes looks good, but failure of the CI needs to be investigated. Clara (L1)

added review-user-action-required label and removed review-pending-level-1 label

mentioned in merge request !34113 (closed)

Hi @sshaw , In this MR we remove from MuonAlignmentCondAlg the updateAlignment calls. This calls were left in to facilitate the migration of the code in 1 thread, but they are not MT safe when going to >1 threads. When removing these calls the MuonDetectorManager in the detector store is not updated anymore for alignment. This causes the test_trig_data_v1Dev_build.py test to fail when comparing the output file with the reference one. In a different MR (!34113 (closed)) we tried to modify the Trigger sw to work with the MuonDetectorManager from the conditions store but this doesn't work since RPC/TGCRecRoiSvc are services and do not participate the scheduling of Read/Write(Cond)Handle(Key). So, the question to you is: Is it important the alignment for the Trigger performance, or should we proceed with this MR by simply updating the reference file?

Hi @stavrop,

do you have somewhere what the differences are (the CI results are no longer available). I can imagine that it's possible this change could change slightly the counts, but wouldn't expect it to be a very large change. Would really need to see the differences first to say for sure it's OK.

(Just to note that in your other MR, TrigMuonEF and the data preparation code in L2MuonSA will not be used in Run 3, so it doesn't matter that they use something that is not thread safe).

Cheers, Savanna

Hi @sshaw , Have a look at /afs/cern.ch/user/s/stavrop/public/run_030eb3d2

Thanks, George. I think this is only affecting one event (though it's small statistics). I think this is probably fine to update the reference, since this likely should cause some small change in the output. Will keep an eye on our nightly tests with larger statistics once this is merged to double check it doesn't cause some unexpectedly large effect.

Just a heads up that there have been other changes to muon triggers in this reference file recently, so you will probably want to update your branch and run the test in the recent nightly so that you get a correct version of the reference to update.

Cheers,

Savanna

added 946 commits

• d618956a...1ce6da54 - 944 commits from branch atlas:master
• 0ad5aac0 - Merge branch 'master' of ssh://gitlab.cern.ch:7999/atlas/athena into rec5533
• db85aee6 - leave A and C lines for now

Compare with previous version

This merge request affects 1 package:

• MuonSpectrometer/MuonConditions/MuonCondGeneral/MuonCondAlg

Adding @rosati ,@nkoehler ,@wleight as watchers

added review-pending-level-1 label and removed review-user-action-required label

Hi @sshaw, @stavrop,

now I left the A lines and C lines update and the test succeeds. So I propose to first merge this, and let me check then, why updating the A/C-lines causes differences.

Best, Nico

CI Result FAILURE (hash db85aee6)

	Athena	AthSimulation	AnalysisBase	AthGeneration
externals
cmake
make
required tests
optional tests				N/A

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 16097]