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

Extend thinning to support slimming #31100

Merged
merged 22 commits into from Sep 2, 2020
Merged

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Aug 8, 2020

PR description:

This PR extends the framework's thinning feature to support slimming as outlined in #30544 (comment):

  • If there are both thinned and thinned+slimmed collections present, a de-reference of Ref-to-parent goes always only to the thinned collections
    • Even if none of the thinned collections contain the requested element (in which case nullptr is returned) and the thinned+slimmed collection would contain the requested element
    • I.e. Ref de-reference means
      • if parent collection is available, return element in it
      • else if any thinned collection (branch) exists
        • if a thinned collection containing the element is available, return the element
        • else return nullptr
      • else if a slimmed collection (branch) exists, take the first slimmed collection searching downward through the parentage tree of thinned/slimmed collections, and apply this algorithm recursively with the slimmed collection as the parent collection
      • else return nullptr
  • The parentage tree of thinned and thinned+slimmed collections may have at most one path from the root (parent) collection to any leaf collection that has slimmed collections visible in that job
    • i.e. one can do e.g. parent -> slimmed1 -> thinned2 -> slimmed3, but not any of
      parent --> slimmed11
             \-> slimmed21
      
      parent --> thinned11 -> slimmed12
             \-> thinned21 -> slimmed22
      
      parent --> thinned11 -> slimmed12
             \-> slimmed21 -> thinned22
      
    • Notably having slimmed1X and slimmed2Y modules in separate jobs is allowed, assuming the other slimmed products are not visible there.
      • Trying to read both slimmed1X and slimmed2Y from different files (via primary and secondary inputs) results an error

In addition, this PR

PR validation:

Framework unit tests pass.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2020

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31100/17650

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2020

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31100/17651

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2020

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

DataFormats/Common
DataFormats/FWLite
DataFormats/Provenance
DataFormats/TestObjects
FWCore/FWLite
FWCore/Framework
FWCore/Integration
IOPool/Streamer

@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@rovere, @wddgit this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

makortel commented Aug 8, 2020

@cmsbuild, please test

@makortel
Copy link
Contributor Author

makortel commented Aug 8, 2020

@Dr15Jones @wddgit Please review

@makortel
Copy link
Contributor Author

makortel commented Aug 8, 2020

FYI @bendavid

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 8, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609667
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2609644
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2020

+1
Tested at: b1c8f44
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-58f66a/9041/summary.html
CMSSW: CMSSW_11_2_X_2020-09-01-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2020

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2609667
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2609638
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

@makortel
Copy link
Contributor Author

makortel commented Sep 2, 2020

+1

I'll collect the remaining items into an issue.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2020

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@makortel
Copy link
Contributor Author

makortel commented Sep 2, 2020

@slava77 @jpata @bendavid Just to make sure, following the discussion in ORP I'm not going to backport this PR (and #30906) to 10_6_X. If the plans change and a backport of these developments would be used in 10_6_X, I can look into a backport (technically should be straightforward).

@makortel
Copy link
Contributor Author

makortel commented Sep 2, 2020

I'll collect the remaining items into an issue.

Done in #31321.

@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit d4d452e into cms-sw:master Sep 2, 2020
@makortel makortel deleted the thinningSlimming branch September 2, 2020 13:12
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

6 participants