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.

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

Circular dependencies in DataFormats #31019

Open
davidlange6 opened this issue Aug 2, 2020 · 31 comments
Open

Circular dependencies in DataFormats #31019

davidlange6 opened this issue Aug 2, 2020 · 31 comments

Comments

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 2, 2020

From pr #31005 by adding the dependencies needed according to #includes, scram finds a number of circular dependencies. @Dr15Jones has fixed one. Here are some others... I inlined what I learned before stopping..

gmake: Circular lib/slc7_amd64_gcc820/DataFormatsSiPixelDetId_xr_rdict.pcm <- tmp/slc7_amd64_gcc820/src/DataFormats/SiPixelDetId/src/DataFormatsSiPixelDetId/a/DataFormatsSiPixelDetId_xr.cc dependency dropped.

Pixel*Name classes should be moved ?

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/SiStripCluster/src/DataFormatsSiStripCluster/a/DataFormatsSiStripCluster_xr.cc <- tmp/slc7_amd64_gcc820/rootpcms/DataFormatsSiStripDetId dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TrackCandidate/src/DataFormatsTrackCandidate/a/DataFormatsTrackCandidate_xr.cc <- tmp/slc7_amd64_gcc820/rootpcms/DataFormatsTrackReco dependency dropped.

This looks simple to fix. Move TrajectoryStopReason

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TrackerCommon/src/DataFormatsTrackerCommon/libDataFormatsTrackerCommon.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsSiStripCluster dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TrackCandidate/src/DataFormatsTrackCandidate/libDataFormatsTrackCandidate.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackCandidate dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/HLTReco/src/DataFormatsHLTReco/libDataFormatsHLTReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsEgammaCandidates dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/HLTReco/src/DataFormatsHLTReco/libDataFormatsHLTReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsJetReco dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/METReco/src/DataFormatsMETReco/libDataFormatsMETReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsEgammaReco dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/METReco/src/DataFormatsMETReco/libDataFormatsMETReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsJetReco dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/RecoCandidate/src/DataFormatsRecoCandidate/libDataFormatsRecoCandidate.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsEgammaReco dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/GsfTrackReco/src/DataFormatsGsfTrackReco/libDataFormatsGsfTrackReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackCandidate dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/GsfTrackReco/src/DataFormatsGsfTrackReco/libDataFormatsGsfTrackReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackingRecHit dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TrajectorySeed/src/DataFormatsTrajectorySeed/libDataFormatsTrajectorySeed.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackingRecHit dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TrajectorySeed/src/DataFormatsTrajectorySeed/libDataFormatsTrajectorySeed.so <- tmp/slc7_amd64_gcc820/cache/prod/libGeometryCommonTopologies dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/RecoCandidate/src/DataFormatsRecoCandidate/libDataFormatsRecoCandidate.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackCandidate dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/RecoCandidate/src/DataFormatsRecoCandidate/libDataFormatsRecoCandidate.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackingRecHit dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/METReco/src/DataFormatsMETReco/libDataFormatsMETReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackCandidate dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/METReco/src/DataFormatsMETReco/libDataFormatsMETReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackingRecHit dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/VertexReco/src/DataFormatsVertexReco/libDataFormatsVertexReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackCandidate dependency dropped
.
gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/VertexReco/src/DataFormatsVertexReco/libDataFormatsVertexReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackingRecHit dependency dropped
.
gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TauReco/src/DataFormatsTauReco/libDataFormatsTauReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsJetReco dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TauReco/src/DataFormatsTauReco/libDataFormatsTauReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsParticleFlowCandidate dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TauReco/src/DataFormatsTauReco/libDataFormatsTauReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackCandidate dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TauReco/src/DataFormatsTauReco/libDataFormatsTauReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackingRecHit dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/HLTReco/src/DataFormatsHLTReco/libDataFormatsHLTReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackCandidate dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/HLTReco/src/DataFormatsHLTReco/libDataFormatsHLTReco.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsTrackingRecHit dependency dropped.

gmake: Circular tmp/slc7_amd64_gcc820/src/Geometry/CommonTopologies/src/GeometryCommonTopologies/libGeometryCommonTopologies.so <- tmp/slc7_amd64_gcc820/cache/prod/libDataFormatsJetReco dependency dropped.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 2, 2020

A new Issue was created by @davidlange6 David Lange.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

assign reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 2, 2020

New categories assigned: reconstruction

@slava77,@perrotta,@jpata you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Aug 2, 2020

the description is pretty unreadable.
Can this be reparsed to something that could be passed to developers? At this point only the first few with explicit comments are only somewhat clear.

@davidlange6
Copy link
Contributor Author

I fixed up some incorrect line breaks - but right, presumably some digging is needed to know the potential solution to each one. I provided the digging that I have done so far

@slava77
Copy link
Contributor

slava77 commented Aug 2, 2020

I fixed up some incorrect line breaks - but right, presumably some digging is needed to know the potential solution to each one. I provided the digging that I have done so far

thanks, this is a bit better already.

Does this provide the same information as in the ignominy (IB QA)?

e.g. https://cmssdt.cern.ch/SDT/cgi-bin/newQA.py?arch=slc7_amd64_gcc820&release=CMSSW_11_2_X_2020-07-30-2300

     Cycle 6
        DataFormats/SiPixelDetId
        DataFormats/SiStripCluster
        DataFormats/SiStripDetId
        DataFormats/TrackerCommon

Can ignominy be used to get a more readable/detailed information?

@davidlange6
Copy link
Contributor Author

davidlange6 commented Aug 2, 2020 via email

@slava77
Copy link
Contributor

slava77 commented Aug 2, 2020

since the issue is not reported in an obvious way,
how to reproduce the report to see that a specific item is gone?

@davidlange6
Copy link
Contributor Author

davidlange6 commented Aug 2, 2020 via email

@Dr15Jones
Copy link
Contributor

The dependencies appear to be a bit more complex

[root@25fa425c5ca5 CMSSW_11_2_0_pre2]# grep TrackerCommon $CMSSW_RELEASE_BASE/src/DataFormats/SiPixelDetId/*/*
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/SiPixelDetId/src/PixelBarrelName.cc:#include "DataFormats/TrackerCommon/interface/TrackerTopology.h"
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/SiPixelDetId/src/PixelEndcapName.cc:#include "DataFormats/TrackerCommon/interface/TrackerTopology.h"
[root@25fa425c5ca5 CMSSW_11_2_0_pre2]# grep TrackerCommon $CMSSW_RELEASE_BASE/src/DataFormats/SiStripDetId/*/*
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/SiStripDetId/interface/SiStripDetId.h:#include "DataFormats/TrackerCommon/interface/SiStripEnums.h"
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/SiStripDetId/interface/StripSubdetector.h:#include "DataFormats/TrackerCommon/interface/SiStripEnums.h"
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/SiStripDetId/src/SiStripSubStructure.cc:#include "DataFormats/TrackerCommon/interface/TrackerTopology.h"
[root@25fa425c5ca5 CMSSW_11_2_0_pre2]# grep SiStripDetId $CMSSW_RELEASE_BASE/src/DataFormats/TrackerCommon/*/*
[root@25fa425c5ca5 CMSSW_11_2_0_pre2]# grep SiPixelDetId $CMSSW_RELEASE_BASE/src/DataFormats/TrackerCommon/*/*
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/TrackerCommon/interface/ClusterSummary.h:#include "DataFormats/SiPixelDetId/interface/PixelSubdetector.h"
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/TrackerCommon/interface/ClusterSummary.h:#include "DataFormats/SiPixelDetId/interface/PixelBarrelName.h"
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/TrackerCommon/interface/TrackerTopology.h:#include "DataFormats/SiPixelDetId/interface/PixelSubdetector.h"
/cvmfs/cms.cern.ch/slc7_amd64_gcc820/cms/cmssw/CMSSW_11_2_0_pre2/src/DataFormats/TrackerCommon/src/TrackerTopology.cc:#include "DataFormats/SiPixelDetId/interface/PixelSubdetector.h"

@davidlange6
Copy link
Contributor Author

davidlange6 commented Aug 2, 2020 via email

@makortel
Copy link
Contributor

makortel commented Aug 3, 2020

gmake: Circular tmp/slc7_amd64_gcc820/src/DataFormats/TrackCandidate/src/DataFormatsTrackCandidate/a/DataFormatsTrackCandidate_xr.cc <- tmp/slc7_amd64_gcc820/rootpcms/DataFormatsTrackReco dependency dropped.

This looks simple to fix. Move TrajectoryStopReason

Took a look because of personal history. Seems to me that the only reason DataFormats/TrackCandidate is used in DataFormats/TrackReco is for dictionary declarations

<class name="std::pair<TrackCandidate,std::pair<reco::Track,reco::Track> >" />
<class name="edm::Wrapper<std::pair<TrackCandidate,std::pair<reco::Track,reco::Track> > >" />
<class name="std::vector<std::pair<TrackCandidate,std::pair<reco::Track,reco::Track> > >"/>
<class name="edm::Wrapper<std::vector<std::pair<TrackCandidate,std::pair<reco::Track,reco::Track> > > >"/>

<class name="std::pair<TrackCandidate,std::pair<edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> >,edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> > > >" />
<class name="edm::Wrapper<std::pair<TrackCandidate,std::pair<edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> >,edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> > > > >" />
<class name="std::vector<std::pair<TrackCandidate,std::pair<edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> >,edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> > > > >"/>
<class name="edm::Wrapper<std::vector<std::pair<TrackCandidate,std::pair<edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> >,edm::Ref<std::vector<reco::Track>,reco::Track,edm::refhelper::FindUsingAdvance<std::vector<reco::Track>,reco::Track> > > > > >"/>

so an alternative could be to move those instead. On the other hand TrackReco has more dependencies than TrackCandidate, which would favor moving TrajectoryStopReason.h.

@davidlange6
Copy link
Contributor Author

I've fixed the TrackReco/Candidate item.

@slava77
Copy link
Contributor

slava77 commented Sep 5, 2020

@davidlange6
can I conclude based on the ignominy report https://cmssdt.cern.ch/SDT/cgi-bin/newQA.py?arch=slc7_amd64_gcc820&release=CMSSW_11_2_X_2020-09-04-2300
that we are done and this issue can be closed?
Or do you see some more issue detected by some other way?

@davidlange6
Copy link
Contributor Author

davidlange6 commented Sep 5, 2020 via email

@slava77
Copy link
Contributor

slava77 commented Sep 5, 2020

Apparently one needs a full build to get a reliable report. I know of at least one more (which is typically reported in the IB q/a information)

https://cmssdt.cern.ch/SDT/cgi-bin/newQA.py?arch=slc7_amd64_gcc10&release=CMSSW_11_2_X_2020-09-04-2300
from a gcc10 full build
indeed, we still have plenty (4?) cycles with DataFormats included.
Screen Shot 2020-09-05 at 8 20 14 AM

@davidlange6
Copy link
Contributor Author

davidlange6 commented Sep 5, 2020 via email

@slava77
Copy link
Contributor

slava77 commented Sep 5, 2020

Cycle 7 is not one I’ve seen before (Eg I didn’t notice it on Thursday). Any recent PR?

I think it's already in CMSSW_11_2_X_2020-08-30-0000
slc7_amd64_gcc820
https://cmssdt.cern.ch/SDT/cgi-bin/newQA.py?arch=slc7_amd64_gcc820&release=CMSSW_11_2_X_2020-08-30-0000
Screen Shot 2020-09-05 at 9 08 06 AM

@davidlange6
Copy link
Contributor Author

davidlange6 commented Sep 5, 2020 via email

@Dr15Jones
Copy link
Contributor

I can't find a connection between JetMETCorrections/FFTJetObjects and Geometry/CommonTopologies. I did

> git grep Geometry | grep JetMETCorrections/FFTJetObjects
> git grep JetMETCorrections | grep Geometry/CommonTopologies

I did that in today's master branch.

@slava77
Copy link
Contributor

slava77 commented Sep 5, 2020

If this is a package level cycle (not the same include file), here is one example:

  • CondFormats/Alignment/src/headers.h depends on CondFormats/External/interface/CLHEP.h
    • CondFormats/External/interface/FFTJet.h includes JetMETCorrections/FFTJetObjects/interface/FFTJetCorrectorSequence.h
      • JetMETCorrections/FFTJetObjects/interface/FFTJetCorrectionsTypemap.h includes DataFormats/JetReco/interface/JPTJet.h
        • DataFormats/JetReco/interface/JPTJet.h includes DataFormats/TrackReco/interface/Track.h
          • DataFormats/TrackReco/interface/HitPattern.h includes DataFormats/TrackingRecHit/interface/TrackingRecHit.h (also Track.h includes TrackBase.h, which includes HitPattern.h)
            • DataFormats/TrackingRecHit/interface/TrackingRecHit.h includes Geometry/CommonDetUnit/interface/GeomDet.h = Geometry/CommonTopologies/interface/GeomDet.h
              • Geometry/CommonTopologies/interface/GeometryAligner.h includes CondFormats/Alignment/interface/Alignments.h

is this a source of the problem?

@davidlange6
Copy link
Contributor Author

so this step is naively the problem:
"CondFormats/External/interface/FFTJet.h includes JetMETCorrections/FFTJetObjects/interface/FFTJetCorrectorSequence.h"

@davidlange6
Copy link
Contributor Author

Seems like FFTJet.h should just move to CondFormats/JetMETObjects. Not sure that will fix everything or not.. (JetMETCorrections/FFTJetObjects seems to be combo of condformats and es producers)

@davidlange6
Copy link
Contributor Author

I proposed it - #31370

@slava77
Copy link
Contributor

slava77 commented Feb 11, 2021

from https://cmssdt.cern.ch/SDT/cgi-bin/newQA.py?arch=slc7_amd64_gcc900&release=CMSSW_11_3_X_2021-02-11-1100#ignominy

Screen Shot 2021-02-11 at 3 48 50 PM

it looks like the cycle was split into two after #31370
Something still remains

@makortel
Copy link
Contributor

makortel commented Feb 12, 2021

It seems to me the cycle 4 in #31019 (comment) is at least partially caused by

typedef edm::AssociationMap<
edm::OneToManyWithQualityGeneric<TrackingParticleCollection, edm::View<reco::Track>, double> >
SimToRecoCollection;
typedef edm::AssociationMap<
edm::OneToManyWithQualityGeneric<edm::View<reco::Track>, TrackingParticleCollection, double> >
RecoToSimCollection;

This file (and corresponding dictionaries) should be moved to SimDataFormats/TrackingAnalysis that already depends on DataFormats/TrackReco.

@perrotta
Copy link
Contributor

perrotta commented Jul 8, 2021

It seems to me the cycle 4 in #31019 (comment) is at least partially caused by

typedef edm::AssociationMap<
edm::OneToManyWithQualityGeneric<TrackingParticleCollection, edm::View<reco::Track>, double> >
SimToRecoCollection;
typedef edm::AssociationMap<
edm::OneToManyWithQualityGeneric<edm::View<reco::Track>, TrackingParticleCollection, double> >
RecoToSimCollection;

This file (and corresponding dictionaries) should be moved to SimDataFormats/TrackingAnalysis that already depends on DataFormats/TrackReco.

This was implemented since last April with #33186

@davidlange6 which is the status of those circular dependencies now?

@slava77
Copy link
Contributor

slava77 commented Jul 8, 2021

I looked a few IBs back but apparently there is no ignominy results available https://cmssdt.cern.ch/SDT/cgi-bin/newQA.py?arch=slc7_amd64_gcc900&release=CMSSW_12_0_X_2021-07-06-0300#ignominy
image

image

@smuzaffar is it still produced?

@smuzaffar
Copy link
Contributor

@slava77 , as we moved to python based SCRAM so ignominy is not running as it was using SCRAM perl modules. I will see if I can update it to use new scram

@slava77
Copy link
Contributor

slava77 commented Jul 20, 2021

@slava77 , as we moved to python based SCRAM so ignominy is not running as it was using SCRAM perl modules. I will see if I can update it to use new scram

@smuzaffar
please let me know if this was checked already, if the ignominy can be expected to run, eventually.

@smuzaffar
Copy link
Contributor

I gave it a look and it is not easy to change ignominy. I will try to port it but it is not on a high priorty list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants