search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.28k forks source link

Circular dependencies in DataFormats #31019

Open davidlange6 opened 4 years ago

davidlange6 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

assign reconstruction

cmsbuild commented 4 years ago

New categories assigned: reconstruction

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

slava77 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

On Aug 2, 2020, at 6:30 PM, Slava Krutelyov notifications@github.com<mailto:notifications@github.com> wrote:

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

Indeed it looks like some of these are reported by ignominy as well. Scram seems to have picked up more after BuildFIles that match what’s #included are used.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/31019#issuecomment-667695505, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ47CJZCKY4XFTI7MCTR6WICXANCNFSM4PSNHS3A.

slava77 commented 4 years ago

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

davidlange6 commented 4 years ago

currently one can do

git cms-merge-topic 31005 scram b

On Aug 2, 2020, at 6:40 PM, Slava Krutelyov notifications@github.com<mailto:notifications@github.com> wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/31019#issuecomment-667696598, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ76BKZWVIWOPBXDQP3R6WJIRANCNFSM4PSNHS3A.

Dr15Jones commented 4 years ago

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 commented 4 years ago

Or probably

Doing grep PackageA PackageB/BuildFile.xml PackageB/interface/ PackageB/src/

And vice versa is sufficient for most I suspect..

On Aug 2, 2020, at 6:43 PM, David Lange notifications@github.com<mailto:notifications@github.com> wrote:

currently one can do

git cms-merge-topic 31005 scram b

On Aug 2, 2020, at 6:40 PM, Slava Krutelyov notifications@github.com<mailto:notifications@github.commailto:notifications@github.com> wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/31019#issuecomment-667696598, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ76BKZWVIWOPBXDQP3R6WJIRANCNFSM4PSNHS3A.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/31019#issuecomment-667696901, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQZ7XATBQIGKYXKLLCTR6WJTJANCNFSM4PSNHS3A.

makortel commented 4 years ago

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 https://github.com/cms-sw/cmssw/blob/e1d765803495f38b2ea45e9f563508012ea8185c/DataFormats/TrackReco/src/classes_def.xml#L476-L479 https://github.com/cms-sw/cmssw/blob/e1d765803495f38b2ea45e9f563508012ea8185c/DataFormats/TrackReco/src/classes_def.xml#L483-L486 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 commented 4 years ago

I've fixed the TrackReco/Candidate item.

slava77 commented 4 years ago

@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 commented 4 years ago

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)

On Sep 5, 2020, at 3:43 AM, Slava Krutelyov notifications@github.com<mailto:notifications@github.com> wrote:

@davidlange6https://github.com/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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/31019#issuecomment-687514656, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ5CM4UQQEWRFU5B4HTSEGJUPANCNFSM4PSNHS3A.

slava77 commented 3 years ago

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 commented 3 years ago

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

On 5 Sep 2020, at 17:22, Slava Krutelyov notifications@github.com wrote:



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]https://user-images.githubusercontent.com/4676718/92308176-ac93a200-ef50-11ea-9625-2129285e09a3.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/31019#issuecomment-687625026, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQ5WN7NXH6TGOHSEGCLSEJJRPANCNFSM4PSNHS3A.

slava77 commented 3 years ago

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 commented 3 years ago

Thx. Probably I was looking at patch builds:(

Cheers, David

On 5 Sep 2020, at 18:09, Slava Krutelyov notifications@github.com wrote:



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]https://user-images.githubusercontent.com/4676718/92309094-57a75a00-ef57-11ea-91c9-b82c4b268c4a.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/31019#issuecomment-687630283, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGPFQY2QAQQQRY34LCKPALSEJPBJANCNFSM4PSNHS3A.

Dr15Jones commented 3 years ago

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 commented 3 years ago

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

is this a source of the problem?

davidlange6 commented 3 years ago

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

davidlange6 commented 3 years ago

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 commented 3 years ago

I proposed it - #31370

slava77 commented 3 years ago

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 commented 3 years ago

It seems to me the cycle 4 in https://github.com/cms-sw/cmssw/issues/31019#issuecomment-777872525 is at least partially caused by https://github.com/cms-sw/cmssw/blob/3220826ed091fd91f9fcef7c45fe698e2fd4e71a/DataFormats/RecoCandidate/interface/TrackAssociation.h#L14-L19 This file (and corresponding dictionaries) should be moved to SimDataFormats/TrackingAnalysis that already depends on DataFormats/TrackReco.

perrotta commented 3 years ago

It seems to me the cycle 4 in #31019 (comment) is at least partially caused by https://github.com/cms-sw/cmssw/blob/3220826ed091fd91f9fcef7c45fe698e2fd4e71a/DataFormats/RecoCandidate/interface/TrackAssociation.h#L14-L19

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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