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

Move GEMDigi dictionary generating classes #30740

Merged
merged 3 commits into from Jul 21, 2020

Conversation

watson-ij
Copy link
Contributor

PR description:

Move GEMDigi dictionary generating classes from EventFilter/GEMRawToDigi to DataFormats/GEMDigi and update references

Per issue #30586 only DataFormats should have objects generating dictionaries.

@jshlee @quark2

PR validation:

Checked upgrade workflow 11621.0 still completes successfully with the changes. Grep'd through cmssw for references to the moved objects, and ran code-checks.

if this PR is a backport please specify the original PR and why you need to backport that PR:

Before submitting your pull requests, make sure you followed this checklist:

@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-30740/17058

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @watson-ij (Ian J. Watson) for master.

It involves the following packages:

DQM/GEM
DataFormats/GEMDigi
EventFilter/GEMRawToDigi

@perrotta, @andrius-k, @kmaeshima, @schneiml, @civanch, @mdhildreth, @cmsbuild, @jfernan2, @fioriNTU, @slava77, @jpata, @kpedro88 can you please review it and eventually sign? Thanks.
@jshlee, @Martin-Grunewald, @dildick, @rovere this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 16, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+1
Tested at: 82e0697
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6335d8/7998/summary.html
CMSSW: CMSSW_11_2_X_2020-07-15-2300
SCRAM_ARCH: slc7_amd64_gcc820

@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-6335d8/7998/summary.html

Comparison Summary:

  • You potentially added 672 lines to the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2612943
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2612894
  • DQMHistoTests: Total skipped: 48
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 146 log files, 17 edm output root files, 35 DQM output files

@jfernan2
Copy link
Contributor

@watson-ij there seems to be erros due to duplicated dictionaries:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6335d8/7998/dupDict/

@watson-ij
Copy link
Contributor Author

@watson-ij there seems to be erros due to duplicated dictionaries:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6335d8/7998/dupDict/

So, the classes.h and classes_def.xml in EventFilter is deleted with this PR, and those classes are moved into the DataFormats. But it looks like the EventFilter classes_def is still being picked up in the Jenkins build? Not sure if theres a way around that?

@cmsbuild
Copy link
Contributor

Pull request #30740 was updated. @perrotta, @andrius-k, @kmaeshima, @schneiml, @civanch, @mdhildreth, @cmsbuild, @jfernan2, @fioriNTU, @slava77, @jpata, @kpedro88 can you please check and sign again.

@perrotta
Copy link
Contributor

please test with cms-sw/cmsdist#6090

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 17, 2020

The tests are being triggered in jenkins.
Tested with other pull request(s) cms-sw/cmsdist#6090
Test Parameters:

@cmsbuild
Copy link
Contributor

-1

Tested at: 1823c7f

CMSSW: CMSSW_11_2_X_2020-07-16-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9c9976/8040/summary.html

I found follow errors while testing this PR

Failed tests: UnitTests

  • Unit Tests:

I found errors in the following unit tests:

---> test TestConfigDP had ERRORS
---> test TestDQMOfflineConfiguration100 had ERRORS
---> test TestDQMOfflineConfiguration200 had ERRORS
---> test materialBudgetTrackerPlots had ERRORS
---> test TestDQMOfflineConfiguration0 had ERRORS
---> test TestDQMOfflineConfiguration150 had ERRORS
---> test TestDQMOfflineConfiguration50 had ERRORS

@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-9c9976/8040/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: 2612943
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2612894
  • DQMHistoTests: Total skipped: 48
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 146 log files, 17 edm output root files, 35 DQM output files

@slava77
Copy link
Contributor

slava77 commented Jul 17, 2020

+1

for #30740 1823c7f

  • code changes are in line with the PR description and the follow up review
    • the data formats moved from EventFilter/GEMRawToDigi were apparently only lightly reviewed, quite likely due to originally more internal need for these classes. Style should be followed better for the centrally distributed DataFormats. Some cleanup/update for non-persisted types, members, and methods would be useful, to follow http://cms-sw.github.io/cms_coding_rules.html#2--naming-rules-1. @jshlee @watson-ij please take a note for a to-do list.
  • jenkins tests pass and comparisons with the baseline show no differences
    • unit test failures are not related to this PR

@slava77
Copy link
Contributor

slava77 commented Jul 20, 2020

dqm, simulation, and upgrade signatures are still needed here.
Please check.

@kpedro88
Copy link
Contributor

+upgrade

@jfernan2
Copy link
Contributor

+1

@silviodonato
Copy link
Contributor

merge

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

8 participants