Parallelized offline vertexing reconstruction #46663
Conversation
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46663/42589
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
Sorry, seems like when I rebased this to 14_2 there was some mix up on my side with the validation branch. Is it ok to push on this branch or do I close and open a new PR from the proper one? |
|
You can push to the same branch. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46663/42721
|
|
A new Pull Request was created by @cericeci for master. It involves the following packages:
The following packages do not have a category, yet: DataFormats/PortableVertex @cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Thanks @makortel. As I had to rebase the proper branch to 14_2 anyhow I ended up updating cherrypicking changes by hand and updating this one. It is now the correct code |
|
assign heterogeneous |
do we need a new subsystem? Elsewhere |
| #include <Eigen/Core> | ||
| #include <Eigen/Dense> |
There was a problem hiding this comment.
please include these only once, before the SoA classes
| #include <Eigen/Core> | |
| #include <Eigen/Dense> |
| using VertexToTrack = Eigen::Vector<float, 1024>; | ||
| using VertexToTrackInt = Eigen::Vector<int, 1024>; |
There was a problem hiding this comment.
Does this mean that the code expects up to 1024 tracks per vertex ?
Does this value appear anywhere else in the code ?
If that is the case, could you define a named constant for it and use it here and everywhere else the same value is needed ?
This should make it easier to update the code if later it turns out that we need a different value.
|
|
||
| using VertexSoA = VertexSoALayout<>; | ||
|
|
||
| using TrackToVertex = Eigen::Vector<float, 512>; // 512 is the max vertex allowed |
There was a problem hiding this comment.
See above, same for 512 here.
There was a problem hiding this comment.
Could you split this into three files, one per SoA, and name them after each SoA ?
There was a problem hiding this comment.
Then I guess the same should be done for VertexHostCollection.h and VertexDeviceCollection.h.
Opinions ?
There was a problem hiding this comment.
I want to first understand if TrackSoALayout and ClusterParams are needed as event data formats, or if they are really only internal details of the PrimaryVertexProducer_Alpaka.
If kept here, on a first though I'd be in favor of splitting into a file per SoA.
There was a problem hiding this comment.
I'm wondering if the content of this new package could instead go into DataFormats/VertexSoA ?
| #include <Eigen/Core> | ||
| #include <Eigen/Dense> | ||
|
|
||
| namespace portablevertex { |
There was a problem hiding this comment.
Do we actually need to introduce a new namespace ?
Would it be a problem to have VertexSoA etc in the global namespace ?
| <lcgdict> | ||
| <class name="alpaka_cuda_async::portablevertex::VertexDeviceCollection" persistent="false"/> | ||
| <class name="edm::DeviceProduct<alpaka_cuda_async::portablevertex::VertexDeviceCollection>" persistent="false"/> | ||
| <class name="edm::Wrapper<edm::DeviceProduct<alpaka_cuda_async::portablevertex::VertexDeviceCollection>>" persistent="false"/> |
There was a problem hiding this comment.
Just to be easier on the eyes, could you add an empty line between each group of collection, device product and wrapper ?
| <read | ||
| sourceClass="portablevertex::VertexHostCollection" | ||
| targetClass="portablevertex::VertexHostCollection" | ||
| version="[1-]" | ||
| source="portablevertex::VertexSoA layout_;" | ||
| target="buffer_,layout_,view_" | ||
| embed="false"> | ||
| <![CDATA[ | ||
| portablevertex::VertexHostCollection::ROOTReadStreamer(newObj, onfile.layout_); | ||
| ]]> | ||
| </read> |
There was a problem hiding this comment.
Please replace the use of explicit read rules with the macro-based approach.
See for example the definitions for portabletest::TestHostCollection in DataFormats/PortableTestObjects/src/classes_def.xml and DataFormats/PortableTestObjects/src/classes.cc
There was a problem hiding this comment.
These changes seem unnecessary ?
If you could avoid touching this file, it would make the impact of the PR smaller.
Thanks @cericeci ! Given that the commit history does not seem very relevant, could you squash all commits into a single one, and add at the bottom of the commit message ? |
In this case the compilation time is very long (IIRC tens of minutes, for reasons not yet fully understood), so a separate package could make sense. The set of dependencies is also somewhat (about 50 %) different. |
| SOA_EIGEN_COLUMN(VertexToTrackInt, track_id), | ||
| SOA_EIGEN_COLUMN(VertexToTrack, track_weight), |
There was a problem hiding this comment.
Do I understand correctly that each vertex element holds weight and index to a track for up to 1024 tracks (i.e. every vertex uses 8 kB memory for the weight and index)?
| <use name="alpaka"/> | ||
| <use name="fmt"/> | ||
| <use name="DataFormats/PortableVertex"/> | ||
| <use name="DataFormats/TrackReco"/> | ||
| <use name="DataFormats/VertexReco"/> | ||
| <use name="FWCore/Framework"/> | ||
| <use name="FWCore/MessageLogger"/> | ||
| <use name="FWCore/ParameterSet"/> | ||
| <use name="FWCore/Utilities"/> | ||
| <use name="HeterogeneousCore/CUDACore"/> | ||
| <use name="HeterogeneousCore/AlpakaTest"/> | ||
| <use name="HeterogeneousCore/AlpakaCore"/> | ||
| <use name="HeterogeneousCore/AlpakaInterface"/> | ||
| <use name="TrackingTools/Records"/> | ||
| <use name="TrackingTools/TransientTrack"/> |
There was a problem hiding this comment.
This list should be driven by what SoAToRecoVertexProducer depends on. By quick look it should be
| <use name="alpaka"/> | |
| <use name="fmt"/> | |
| <use name="DataFormats/PortableVertex"/> | |
| <use name="DataFormats/TrackReco"/> | |
| <use name="DataFormats/VertexReco"/> | |
| <use name="FWCore/Framework"/> | |
| <use name="FWCore/MessageLogger"/> | |
| <use name="FWCore/ParameterSet"/> | |
| <use name="FWCore/Utilities"/> | |
| <use name="HeterogeneousCore/CUDACore"/> | |
| <use name="HeterogeneousCore/AlpakaTest"/> | |
| <use name="HeterogeneousCore/AlpakaCore"/> | |
| <use name="HeterogeneousCore/AlpakaInterface"/> | |
| <use name="TrackingTools/Records"/> | |
| <use name="TrackingTools/TransientTrack"/> | |
| <use name="DataFormats/Math"/> | |
| <use name="DataFormats/PortableVertex"/> | |
| <use name="DataFormats/TrackReco"/> | |
| <use name="DataFormats/VertexReco"/> | |
| <use name="FWCore/Framework"/> | |
| <use name="FWCore/ParameterSet"/> | |
| <use name="FWCore/Utilities"/> |
| <use name="FWCore/Framework"/> | ||
| <use name="FWCore/ParameterSet"/> | ||
| <use name="FWCore/Utilities"/> | ||
| <use name="HeterogeneousCore/CUDACore"/> |
There was a problem hiding this comment.
This dependence should not be needed
| <use name="HeterogeneousCore/CUDACore"/> |
| #include "FWCore/Framework/interface/stream/EDProducer.h" | ||
| #include "FWCore/Framework/interface/Event.h" | ||
| #include "FWCore/Framework/interface/EventSetup.h" | ||
| #include "HeterogeneousCore/AlpakaInterface/interface/config.h" |
There was a problem hiding this comment.
Should not be needed
| #include "HeterogeneousCore/AlpakaInterface/interface/config.h" |
| // Finally, add references to the reco::Track used for building it | ||
| for (int iT=0; iT < hostVertexView[iV].ntracks(); iT++) { | ||
| int new_itrack = hostVertexView[iV].track_id()[iT]; | ||
| reco::TrackRef ref(tracks, new_itrack); |
There was a problem hiding this comment.
Not sure where to put this comment, but it should be ensured in some way the ProductID of the edm::Handle<reco::TrackCollection> is the same as what was used for hostVertexView.
| @@ -0,0 +1,166 @@ | |||
| #include "DataFormats/PortableVertex/interface/alpaka/VertexDeviceCollection.h" | |||
| #include "DataFormats/PortableVertex/interface/VertexHostCollection.h" | |||
| #include "DataFormats/BeamSpot/interface/BeamSpotHost.h" | |||
There was a problem hiding this comment.
I believe this should be
| #include "DataFormats/BeamSpot/interface/BeamSpotHost.h" | |
| #include "DataFormats/BeamSpot/interface/alpaka/BeamSpotDevice.h" |
| @@ -0,0 +1,166 @@ | |||
| #include "DataFormats/PortableVertex/interface/alpaka/VertexDeviceCollection.h" | |||
| #include "DataFormats/PortableVertex/interface/VertexHostCollection.h" | |||
There was a problem hiding this comment.
This should not be needed
| #include "DataFormats/PortableVertex/interface/VertexHostCollection.h" |
| #include "DataFormats/VertexReco/interface/VertexFwd.h" | ||
| #include "DataFormats/TrackReco/interface/TrackFwd.h" | ||
| #include "DataFormats/TrackReco/interface/Track.h" | ||
| #include "DataFormats/VertexReco/interface/Vertex.h" |
There was a problem hiding this comment.
Are these four headers needed? I didn't see any obvious use for them
| #include "DataFormats/VertexReco/interface/VertexFwd.h" | |
| #include "DataFormats/TrackReco/interface/TrackFwd.h" | |
| #include "DataFormats/TrackReco/interface/Track.h" | |
| #include "DataFormats/VertexReco/interface/Vertex.h" |
|
|
||
| cmsRun testPrimaryVertexProducer_Alpaka.py --backend $i | ||
|
|
||
| python compareAlgos.py testAlpaka.root testCPU.root |
There was a problem hiding this comment.
This script alone seems to not to be very useful. The files
../PrimaryVertexProducer/test/testPrimaryVertexProducer_CPU.pytestPrimaryVertexProducer_Alpaka.pycompareAlgos.py
Are these files perhaps just missing from the PR?
|
|
||
| using TrackSoA = TrackSoALayout<>; | ||
|
|
||
| GENERATE_SOA_LAYOUT(ClusterParams, |
There was a problem hiding this comment.
It looks like the ClusterParams is used only internally in PrimaryVertexProducer_Alpaka, and is not put into the Event. Then it would be better defined in RecoVertex/PrimaryVertexProducer_Alpaka/plugins/alpaka.
|
+1 Size: This PR adds an extra 56KB to repository Comparison SummarySummary:
GPU Comparison SummarySummary:
|
|
enable profiling |
Either way the name |
That I fully agree (and kind of already commented in #46663 (comment)). In case of a separate package, I think the package name should match the (main) producer name. We have some existing use of a |
|
@smuzaffar Do we have any monitoring of the compilation time of a PR? |
@jfernan2 Just curious, why |
If PR job is still in jenkins then yes we can find the time it took for compilation otherwise we can only get the total time it took run the PR test job. By the way, compilation time can vary depending on what other packages were checkout |
|
@smuzaffar Would it be feasible to run the (by the way, why do I see |
|
@makortel , sure I can add |
|
@makortel , ok I think I know why we have duplicate compilation messages. Bot runs so we get two compilation messages. One directly from scram and 2nd from the The third message is coming from https://github.com/cms-sw/cms-bot/blob/master/pr_testing/test_multiple_prs.sh#L1149 where we try to get any log messages where were in tmp logs files but were not printed ( in case of build failures). I will update bot to avoid using |
|
Thanks @smuzaffar! |
You are right @makortel I did not realize there is no configuration added into the PR, I was eager to find a candidate PR to test the new profiling script. I am sorry |
|
enable none |
ah its not cms-sw/cmsdist#9526 should fix the duplicate build messages |
| iEvent.getHandle(recoTrackToken_) | ||
| .product(); // Note that we need reco::Tracks for building the track Reference vector inside the reco::Vertex | ||
|
|
||
| // This is an annoying conversion as the vertex expects a transient track here, which is a dataformat which we otherwise bypass |
There was a problem hiding this comment.
What does this comment applies to ?
Where are TransientTracks used in this producer ?
|
Milestone for this pull request has been moved to CMSSW_15_0_X. Please open a backport if it should also go in to CMSSW_14_2_X. |
PR description:
This PR contains a first implementation of the parallelized offline vertexing using Alpaka. We intend this as a first PR to request feedback and comments. It includes contributions from @alexstrel that were squashed in during the porting to 14_2.
The content of the PR was previously discussed on:
https://indico.cern.ch/event/1442046/#4-status-of-offline-vertexing
PR validation:
When running the code-checks there are several warnings such as:
which I'm not sure about their origin. Otherwise it properly compiles and (local) validation works. The added code should not touch any other packages.