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

Jump to bottom

Add missing dependencies on TBB #40376

Merged
merged 1 commit into from Jan 10, 2023
Merged

Add missing dependencies on TBB #40376

merged 1 commit into from Jan 10, 2023

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Dec 20, 2022

PR description:

Add missing dependencies on TBB to the BuildFiles.

PR validation:

None.

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 20, 2022

please test

@cmsbuild cmsbuild added this to the CMSSW_13_0_X milestone Dec 20, 2022
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40376/33492

  • This PR adds an extra 40KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

  • CommonTools/Utils (reconstruction)
  • DQM/EcalCommon (dqm)
  • DQMServices/Core (dqm)
  • DQMServices/FwkIO (dqm)
  • DataFormats/Common (core)
  • DataFormats/L1GlobalMuonTrigger (l1)
  • DataFormats/PatCandidates (xpog, reconstruction)
  • EventFilter/Utilities (daq)
  • FWCore/Common (core)
  • FWCore/Framework (core)
  • FWCore/PythonFramework (core)
  • FWCore/Reflection (core)
  • FWCore/Services (core)
  • FWCore/TestProcessor (core)
  • GeneratorInterface/LHEInterface (generators)
  • HLTrigger/Timer (hlt)
  • HeterogeneousCore/AlpakaInterface (heterogeneous)
  • HeterogeneousCore/CUDACore (heterogeneous)
  • HeterogeneousCore/CUDAServices (heterogeneous)
  • IOPool/Output (core)
  • Mixing/Base (simulation)
  • PhysicsTools/NanoAOD (xpog)
  • PhysicsTools/TensorFlow (reconstruction)
  • RecoLocalCalo/HGCalRecProducers (upgrade, reconstruction)
  • RecoTauTag/RecoTau (reconstruction)
  • RecoTracker/CkfPattern (reconstruction)
  • RecoTracker/MkFit (reconstruction)
  • Utilities/XrdAdaptor (core)

@SiewYan, @menglu21, @Saptaparna, @Martin-Grunewald, @rekovic, @alberto-sanchez, @cecilecaillol, @civanch, @vlimant, @makortel, @ahmad3213, @missirol, @fwyzard, @GurpreetSinghChahal, @mandrenguyen, @smorovic, @pmandrik, @smuzaffar, @Dr15Jones, @epalencia, @emanueleusai, @mdhildreth, @AdrianoDee, @syuvivida, @micsucmed, @mkirsano, @swertz, @emeschi, @clacaputo, @srimanob, @rvenditti can you please review it and eventually sign? Thanks.
@gouskos, @VourMa, @felicepantaleo, @argiro, @Martin-Grunewald, @bsunanda, @pfs, @thomreis, @alberto-sanchez, @lgray, @mmusich, @youyingli, @sethzenz, @missirol, @makortel, @JanFSchulte, @mbluj, @dgulhan, @apsallid, @azotz, @simonepigazzini, @beaucero, @vandreev11, @barvic, @trtomei, @GiacomoSguazzoni, @rovere, @VinInn, @cseez, @hatakeyamak, @ebrondol, @mtosi, @fabiocos, @mkirsano, @rchatter, @riga, @wddgit, @edjtscott, @silviodonato, @lecriste, @gpetruc, @sameasy this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 20, 2022

+heterogeneous

makortel
makortel reviewed Dec 20, 2022
View reviewed changes
HeterogeneousCore/AlpakaInterface/BuildFile.xml Outdated
@@ -1,3 +1,4 @@
<use name="alpaka"/>
<use name="boost_header"/>
<use name="tbb"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything depending directly on TBB here. The definition of alpaka_tbb_async backend is there, but it is not used by scram build rules right now. When the time comes to enable the TBB backend, the dependence on tbb external could be treated similarly to other backend-specific externals (like cuda). Practical impact is close to zero though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm... my mistake, I included the comments in my search for tbb:: :-)

RecoTracker/CkfPattern/BuildFile.xml
<use name="FWCore/ParameterSet"/>
<use name="FWCore/MessageLogger"/>
<use name="root"/>
<use name="tbb"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package uses TBB only if VI_TBB macro is defined, which is not by default. I think it would be more natural to leave out the tbb dependence here, even if the practical impact is close to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right... I'll leave a comment in the BuildFile.xml for the next person that relies only on grep to identify the missing dependencies :-)

@fwyzard fwyzard force-pushed the add_missing_dependencies_on_tbb branch from ae23cc3 to dc4ae77 Compare December 20, 2022 21:23
@mandrenguyen
Copy link
Contributor

+reconstruction

@srimanob
Copy link
Contributor

+Upgrade

@emanueleusai
Copy link
Member

+1

  • technical PR

@makortel
Copy link
Contributor

makortel commented Jan 9, 2023

@cmsbuild, please test

Let's refresh

@makortel
Copy link
Contributor

makortel commented Jan 9, 2023

+heterogeneous

@makortel
Copy link
Contributor

makortel commented Jan 9, 2023

@cms-sw/heterogeneous-l2 Could you please review and sign?

@fwyzard
Copy link
Contributor Author

fwyzard commented Jan 9, 2023

@cms-sw/heterogeneous-l2 Could you please review and sign?

I'm confused, didn't you just do it ?

@makortel
Copy link
Contributor

makortel commented Jan 9, 2023

@cms-sw/heterogeneous-l2 Could you please review and sign?

I'm confused, didn't you just do it ?

Yeah, it should have been "@cms-sw/generators-l2 Could you please review and sign". Thanks for point it out.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 9, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0442c2/29845/summary.html
COMMIT: 9311e1b
CMSSW: CMSSW_13_0_X_2023-01-09-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40376/29845/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 5 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3555538
  • DQMHistoTests: Total failures: 154
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3555362
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 211 log files, 162 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

+1

  • @cms-sw/generators-l2 changes in the only file in your area affected are technical and rather straightforward: let merge this PR before it gets unmergeable, and please complain later if you see anything wrong in it

@perrotta
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 8ba73db into cms-sw:master Jan 10, 2023
This was referenced Jan 10, 2023
Merged
Merged
@fwyzard fwyzard deleted the add_missing_dependencies_on_tbb branch March 9, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makortel makortel makortel left review comments

Assignees
No one assigned
Labels
code-checks-approved core-approved daq-approved dqm-approved generators-pending heterogeneous-approved hlt-approved l1-approved orp-approved pending-signatures reconstruction-approved simulation-approved tests-approved upgrade-approved xpog-approved
Projects
None yet
Milestone
CMSSW_13_0_X
Development

Successfully merging this pull request may close these issues.

None yet