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

Provide easier to use ::get and ::getHandle methods for Event, Run and LuminosityBlock #25707

Merged
merged 6 commits into from
Jan 23, 2019

Conversation

Dr15Jones
Copy link
Contributor

By using an edm::EDGetTokenT<T> one can now do

auto const& tracks = event.get(tracksToken);

or if one needs the edm::Handle

auto hTracks = event.getHandle(tracksToken);

Instead of passing BasicHandle in as an argument, we now return it directly from the function. This allowed removal of the default constructor.
Attempted to optimize convert_handle.
Returns directly the object with which one wants to interact.
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25707/8093

  • This PR adds an extra 180KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

DataFormats/Common
FWCore/Common
FWCore/Framework
FWCore/Integration
FWCore/Modules
FWCore/Skeletons
IOPool/Input
IOPool/Output
IOPool/SecondaryInput
IOPool/Streamer

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @rovere, @wddgit this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 19, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/32716/console Started: 2019/01/19 16:58

@cmsbuild
Copy link
Contributor

-1

Tested at: 6d61969

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25707/32716/summary.html

I found follow errors while testing this PR

Failed tests: Build HeaderConsistency

  • Build:

I found compilation error when building:

>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/PreMixingModule/plugins/PreMixingCrossingFrameWorker.cc 
>> Compiling edm plugin /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/PreMixingModule/plugins/PreMixingDigiAccumulatorWorker.cc 
In file included from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/PreMixingModule/plugins/PreMixingPileupCopy.cc:5:0:
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/MixingModule/interface/PileUpEventPrincipal.h: In instantiation of 'bool PileUpEventPrincipal::getByLabel(const edm::InputTag&, edm::Handle&) const [with T = std::vector]':
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/PreMixingModule/plugins/PreMixingPileupCopy.cc:37:57:   required from here
/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/MixingModule/interface/PileUpEventPrincipal.h:46:19: error: no matching function for call to 'convert_handle(std::remove_reference::type, edm::Handle >&)'
     convert_handle(std::move(bh), result);
     ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
In file included from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/FWCore/Framework/interface/Event.h:21:0,
                 from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/PreMixingModule/plugins/PreMixingPileupCopy.h:12,
                 from /build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_5_X_2019-01-18-2300/src/SimGeneral/PreMixingModule/plugins/PreMixingPileupCopy.cc:1:


@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

-1

Tested at: 6d31f49

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25707/32719/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25707/32719/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3097440
  • DQMHistoTests: Total failures: 142
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3097101
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 133 log files, 14 edm output root files, 32 DQM output files

@Dr15Jones
Copy link
Contributor Author

@smuzaffar looks like the unit test failure was from one of the tests never ending

 gmake: *** [config/SCRAM/GMake/Makefile.rules:1735: unittests_ExpressionEvaluatorUnitTest] Terminated

[NOTE: that test makes no use of the code I changed, or even any part of the framework]

@Dr15Jones
Copy link
Contributor Author

+1
The unit test failure is not related to this code
The comparison differences are all from Taus coming from L1 or HLT. If this pull request caused a problem it should have caused problems across all ranges of the code.

@civanch
Copy link
Contributor

civanch commented Jan 22, 2019

+1
hope, that the unit test will be fixed

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@Dr15Jones
Copy link
Contributor Author

@fabiocos what is your thoughts on integrating this? The test failure was not from this change.

@fabiocos
Copy link
Contributor

@Dr15Jones I'll integrate it in next IB, so we may see

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit fc4815a into cms-sw:master Jan 23, 2019
@Dr15Jones Dr15Jones deleted the eventGetHandle branch January 24, 2019 14:28
@smuzaffar
Copy link
Contributor

@Dr15Jones , look like this change has caused 3 compilation warnings in IBs.

@Dr15Jones
Copy link
Contributor Author

@Dr15Jones , look like this change has caused 3 compilation warnings in IBs.

@smuzaffar I actually saw those warnings before this change when I build FWCore/Integration as part of my usual build. I have yet to figure out what the compiler is complaining about.

@Dr15Jones
Copy link
Contributor Author

@smuzaffar I just tracked down what caused that compilation warning. [It is not this pull request.]

@Dr15Jones
Copy link
Contributor Author

@smuzaffar the fix is in #25764

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

Successfully merging this pull request may close these issues.

5 participants