Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move dictionaries (and some classes) defined in SimTracker/TrackTriggerAssociation and SimGeneral/TrackingAnalysis to more proper packages #45567

Merged
merged 9 commits into from
Aug 9, 2024
Merged

Move dictionaries (and some classes) defined in SimTracker/TrackTriggerAssociation and SimGeneral/TrackingAnalysis to more proper packages #45567

merged 9 commits into from
Aug 9, 2024

Conversation

makortel
Copy link
Contributor

PR description:

#45423 (comment) pointed to potential issues in dictionaries defined in SimTracker/TrackTriggerAssociation and SimGeneral/TrackingAnalysis. This PR

  • Moves dictionary of map<string, unsigned long long> to DataFormats/StdDictionaries
  • Removes dictionary of map<int, int> from SimTracker/TrackTriggerAssociation because it is already defined in DataFormats/StdDictionaries
  • Removes dictionaries of map<int, HepMC...> from SimTracker/TrackTriggerAssociation because they are already defined in SimDataFormats/GeneratorProducts
  • Removes TrackingParticle-only dictionaries from SimTracker/TrackTriggerAssociation because they are already defined in SimDataFormats/TrackingAnalysis
  • Moves TT{Cluster,Stub,Track}AssociationMap classes and their dictionaries from SimTracker/TrackTriggerAssociation to SimDataFormats/Associations.
    • I saw in IB RelVal framework job reports these classes are persisted, and therefore they must be defined in a data format package. SimDataFormats/Associations seemed like a reasonable placement.
  • Marks tt::StubAssociation as transient
    • I did not see it being persisted at the moment, and the dependence on tt::Setup (defined in L1Trigger/TrackTrigger) implies it (in its present form) can not be moved to a data format package, and therefore it must not be persisted
  • Moves dictionaries of std::pairs of Ptr<TTTrack> and Ptr<TrackingParticle> from SimGeneral/TrackingAnalysis to SimDataFormats/Associations
  • These seem to be related to the std::maps already defined there (by an earlier commit in this PR)
  • Moves dictionaries of std::pair<TrackingParticleRef, TrackPSimHitRef> etc from SimGeneral/TrackingAnalysis to SimDataFormats/Associations
    • I saw these are persisted, and therefore the dictionaries must be defined in a data format package. SimDataFormats/Associations seemed like a reasonable placement.

I am a little bit concerned though that the SimDataFormats/Associations package has become a kitchen sink, which means that some jobs may end up loading more shared libraries than necessary.

Resolves cms-sw/framework-team#968

PR validation:

The modified packages compiled in 14_1_0_pre5 (I had to rebase afterwards but the conflict looked simple enough).

makortel added 7 commits July 26, 2024 13:38
@makortel
Move dictionary of map<string, unsigned long long> to StdDictionaries
69dd839 
Also remove map<int, int> from SimTracker/TrackTriggerAssociation as
it exists already in StdDictionaries
@makortel
Remove redundant map<int, HepMC...> dictionaries
680e062 
These are already defined in the correct SimDataFormats/GeneratorProducts packages
@makortel
Remove TrackingParticle-only dictionaries from SimTracker/TrackTrigge…
c89ac91 
…rAssociation

These are already defined in SimDataFormats/TrackingAnalysis
@makortel
Move TT{Cluster,Stub,Track}AssociationMap to SimDataFormats/Associations
64cd832 
These classes are persisted, and therefore must be defined in a data
format package. These classes are associations, and therefore
SimDataFormats/Associations seems like a reasonable place, especially
given that it already depends on DataFormats/L1TrackTrigger.
@makortel
Mark tt::StubAssociation as transient
051a261 
The dependence on tt::Setup implies the tt::StubAssociation may not be persisted.
@makortel
Move dictionaries of pair of Ptr<TTrack> and Ptr<TrackingParticle> to…
55a4d74 
… SimDataFormats/Associations

These seem to be related to the maps of those Ptr types alredy defined there.
@makortel
Move pair<TrackingParticleRef, TrackPSimHitRef> dictionaries to SimDa…
058a9c9 
…taFormats/Associations

These are persisted, and therefore must be defined in a data format
package. Since the pair and map are about associating TrackingParticle
to PSimHits, the SimDataFormats/Associations seems like a reasonable
place. An alternative place could have been
SimDataFormats/TrackingAnalysis where TrackingParticle is defined.
@cmsbuild cmsbuild added this to the CMSSW_14_1_X milestone Jul 26, 2024
@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 26, 2024 •
edited
Loading

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45567/41062

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel for master.

It involves the following packages:

  • DQMOffline/L1Trigger (dqm, l1)
  • DataFormats/StdDictionaries (core)
  • L1Trigger/L1TTrackMatch (l1, upgrade)
  • L1Trigger/TrackFindingTMTT (l1)
  • L1Trigger/TrackFindingTracklet (l1)
  • L1Trigger/TrackTrigger (l1, upgrade)
  • L1Trigger/TrackerDTC (l1, upgrade)
  • L1Trigger/VertexFinder (l1)
  • RecoEgamma/EgammaHLTProducers (hlt)
  • SimDataFormats/Associations (simulation)
  • SimGeneral/TrackingAnalysis (simulation)
  • SimTracker/TrackTriggerAssociation (simulation, l1)
  • Validation/SiTrackerPhase2V (dqm)

@Dr15Jones, @Martin-Grunewald, @aloeliger, @antoniovagnerini, @civanch, @cmsbuild, @epalencia, @makortel, @mdhildreth, @mmusich, @nothingface0, @rvenditti, @smuzaffar, @srimanob, @subirsarkar, @syuvivida, @tjavaid can you please review it and eventually sign? Thanks.
@CeliaFernandez, @Fedespring, @GiacomoSguazzoni, @HuguesBrun, @JanFSchulte, @Martin-Grunewald, @Prasant1993, @Sam-Harper, @VinInn, @VourMa, @a-kapoor, @abbiendi, @afiqaize, @andrea21z, @apsallid, @arossi83, @bsunanda, @cericeci, @dgulhan, @erikbutz, @fabiocos, @felicepantaleo, @jainshilpi, @jhgoh, @lgray, @martinamalberti, @missirol, @mmusich, @mtosi, @ram1123, @rociovilar, @rovere, @sameasy, @silviodonato, @skinnari, @slomeo, @sobhatta, @sroychow, @sviret, @threus, @trocino, @valsdav, @varuns23, @wddgit, @youyingli this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@makortel
Copy link
Contributor Author

type -changes-dataformats

@makortel
Copy link
Contributor Author

Comparison differences are related to #39803

@makortel
Copy link
Contributor Author

+core

@Martin-Grunewald
Copy link
Contributor

+1

@civanch
Copy link
Contributor

civanch commented Jul 31, 2024

+1

@srimanob
Copy link
Contributor

srimanob commented Aug 3, 2024

+Upgrade

@tjavaid
Copy link

tjavaid commented Aug 8, 2024

@makortel , I see several duplication of the dictionaries. Do those need a fix ?

@makortel
Copy link
Contributor Author

makortel commented Aug 8, 2024

I see several duplication of the dictionaries. Do those need a fix ?

@tjavaid Those are false positives caused by a deficiency of the checker script. Because the SimGeneral/TrackingAnalysis/src/classes_def.xml is removed in this PR, the checker falls back to finding the file from the release area, and then reports the duplication. These warnings will disappear in a full IB build.

@cmsbuild cmsbuild mentioned this pull request Aug 8, 2024
Open
@tjavaid
Copy link

tjavaid commented Aug 9, 2024

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2024

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @mandrenguyen, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 677d522 into cms-sw:master Aug 9, 2024
11 checks passed
@makortel makortel deleted the fixDictionariesTrackTriggerAssociation branch August 9, 2024 15:30
@cmsbuild cmsbuild mentioned this pull request Aug 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
code-checks-approved core-approved dqm-approved fully-signed hlt-approved l1-approved orp-approved simulation-approved tests-approved tracking trk upgrade-approved
Projects
None yet
Milestone
CMSSW_14_1_X
8 participants
@makortel @cmsbuild @aloeliger @Martin-Grunewald @civanch @srimanob @tjavaid @mandrenguyen