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

Faster provenance handling #28536

Merged
merged 6 commits into from Dec 9, 2019
Merged

Conversation

Dr15Jones
Copy link
Contributor

PR description:

The ProductProvenanceRetriever used to hold the parentage information for the Event data products now uses a faster data structure and avoids resetting itself every Event.

PR validation:

The framework unit tests all pass.

Switching to the use of an std::optional allows the use of std::move in a clear manner.
Denote whether the data product and its provenance will be set by the input source on event read. This means the provenance will use the insertIntoSet method of ProductProvenanceRetriever.
Setup at construction the data structure used to hold all possible BranchIDs that can be called using insertIntoSet. The individual std::vector elements can each be written to by different threads safely. As the structure is re-used, any later changes to the ProductRegistry require updating the structure.
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28536/13009

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 3, 2019

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

It involves the following packages:

DataFormats/Provenance
DataFormats/Streamer
FWCore/Framework
IOPool/Streamer

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @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 Dec 3, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3778/console Started: 2019/12/04 00:20

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

-1

Tested at: a9e7916

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

I found follow errors while testing this PR

Failed tests: ClangBuild

  • Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

Comparison not run due to Build errors/Fireworks only changes/No short matrix requested (RelVals and Igprof tests were also skipped)

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3798/console Started: 2019/12/04 16:56

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

+1
Tested at: 7adf9c9
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e3a77e/3798/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e3a77e/3798/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e3a77e/3798/git-merge-result

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2019

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2793840
  • DQMHistoTests: Total failures: 26
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2793473
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@Dr15Jones
Copy link
Contributor Author

+1
The comparison failures are the same as seen in #28474 which was included by the bot in the testing.

@fabiocos
Copy link
Contributor

fabiocos commented Dec 5, 2019

@smuzaffar here I see another case of tests run against 11_0_X for a 11_1_X PR

@fabiocos
Copy link
Contributor

fabiocos commented Dec 5, 2019

@qliphy @agrohsje this looks a technical update, I am performing a quick cross check on LHE provenance, please have a look, i will integrate this PR ass soon as I have finished

@smuzaffar
Copy link
Contributor

correct, This issue has been identified and fixed. The ib baseline generation job was using your old branch for 11.0.X [a]. This was done to test your changes but it was never reverted. This only effected the generation of baseline job to use an old cms-bot. Note that https://github.com/cms-sw/cms-bot/blob/master/run-ib-comparison-baseline in your branch and cms-bot master is identical, so this did not break any thing till now.

Problem is that in your branch has DEV release set to CMSSW_11_0_X while now we move to CMSSW_11_1_X. Anyway, feel free to re-tests if case you want to get clean

[a]

if [ $(echo $RELEASE_FORMAT | grep 'CMSSW_11_0_X_' | wc -l) -gt 0 ] ; then
  git clone -b fc-IBperf https://github.com/fabiocos/cms-bot
else
  git clone https://github.com/cms-sw/cms-bot
fi

@fabiocos
Copy link
Contributor

fabiocos commented Dec 5, 2019

@smuzaffar thanks, good that it is understood, I'll check and clean my branch

@fabiocos
Copy link
Contributor

fabiocos commented Dec 9, 2019

@qliphy @agrohsje please check, I will integrate this PR in next IB

@qliphy
Copy link
Contributor

qliphy commented Dec 9, 2019

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 9, 2019

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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

fabiocos commented Dec 9, 2019

+1

@cmsbuild cmsbuild merged commit 775cc65 into cms-sw:master Dec 9, 2019
@Dr15Jones Dr15Jones deleted the fasterProvenanceHandling branch December 10, 2019 20:37
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.

None yet

5 participants