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

Few further esConsumes-related updates for CondFormats (RecoMuon and DT) test folders #36065

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

battibass
Copy link

@battibass battibass commented Nov 9, 2021

PR description:

This PR addresses the migration to esConsumes of a few remaining modules, present in the test/ folders of CondFormats/RecoMuonObjects and CondFormats/DTObjects, as reported here and here.

PR validation:

AFAIK the code does not run in any central workflow.
Code compiles, passes code-checks and code-format, and a few cfgs from the test folders were exercised.

@battibass
Copy link
Author

@swagata87

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36065/26533

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2021

A new Pull Request was created by @battibass (Carlo Battilana) for master.

It involves the following packages:

  • CondFormats/DTObjects (db, alca)
  • CondFormats/RecoMuonObjects (db, alca)

@malbouis, @yuanchao, @cmsbuild, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@mmusich, @tocheng, @seemasharmafnal this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@tvami
Copy link
Contributor

tvami commented Nov 9, 2021

@cmsbuild , please test

@tvami
Copy link
Contributor

tvami commented Nov 9, 2021

Some comments on a future improvement:

  • under \test the header files should be merged into the .cc file
  • it's good to add unit tests (i.e. run the py config files)

Reading through the code I also notice that they use the legacy modules... this will need to be changed in this PR, in this state the Jenkins tests will have SA errors

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-310366/20411/summary.html
COMMIT: 9eacc17
CMSSW: CMSSW_12_2_X_2021-11-09-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36065/20411/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 2901890
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2901868
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 41 files compared)
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Nov 9, 2021

Some comments on a future improvement:

  • under \test the header files should be merged into the .cc file
  • it's good to add unit tests (i.e. run the py config files)

Reading through the code I also notice that they use the legacy modules... this will need to be changed in this PR, in this state the Jenkins tests will have SA errors

Ahh yes, the SA doesnt check the \test directory... @battibass do you prefer to have another PR then for the legacy module fix?

@ggovi
Copy link
Contributor

ggovi commented Nov 10, 2021

+1

@battibass
Copy link
Author

Ahh yes, the SA doesn't check the \test directory... @battibass do you prefer to have another PR then for the legacy module fix?

@tvami , yes, let's deal with that in a second PR please.
In general, do you have further instructions about how to check code compliance with CMSSW standards, beyond running scram code-checks and scram code-format?
Other than this, do you have a pointer about the implementation of unit tests in CMSSW?

@tvami
Copy link
Contributor

tvami commented Nov 10, 2021

@battibass can you please also include RecoMuon and DT in the PR title? It's good to be able to figure out what part of CondFormats you are changing just by reading the title

@tvami
Copy link
Contributor

tvami commented Nov 10, 2021

@tvami , yes, let's deal with that in a second PR please.

Ok, I'll sign it now (but please still rename the PR)

In general, do you have further instructions about how to check code compliance with CMSSW standards, beyond running scram code-checks and scram code-format?

Hmm one pointer I can give is this link: https://cms-sw.github.io/cms_coding_rules.html
Let me know if this is what you had in mind.

Other than this, do you have a pointer about the implementation of unit tests in CMSSW?

Sure, I've made a new unit test in this ongoing PR
https://github.com/cms-sw/cmssw/pull/36028/files
the relevant files are

Or another example with a lot of unit tests is in
https://github.com/cms-sw/cmssw/blob/master/CondTools/SiPixel/test/createTestDBObjects.sh

@tvami
Copy link
Contributor

tvami commented Nov 10, 2021

+alca

@cmsbuild
Copy link
Contributor

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

@qliphy qliphy changed the title Few further esConsumes-related updates for CondFormats test folders Few further esConsumes-related updates for CondFormats (RecoMuon and DT) test folders Nov 11, 2021
@qliphy
Copy link
Contributor

qliphy commented Nov 11, 2021

+1

@cmsbuild cmsbuild merged commit 8f97ed4 into cms-sw:master Nov 11, 2021
@battibass
Copy link
Author

@tvami, thanks a lot!

For the unit tests, that is all I need.

About:

Hmm one pointer I can give is this link: https://cms-sw.github.io/cms_coding_rules.html Let me know if this is what you had in mind.

My question was more wether there are additional commands or compilation flag one must use, beyond what I already tested with scam, before submitting the PR.
E.g.: I am not sure that the checks I ran would have spot the lack of esConsumes migration that caused the need for a PR.

@tvami
Copy link
Contributor

tvami commented Nov 11, 2021

I'm not fully sure of the details, there is this way to compile with errors and libchecker, this is done every IB, see the latest here
https://cmssdt.cern.ch/SDT/cgi-bin/showBuildLogs.py/slc7_amd64_gcc900/www/wed/12.2.CMSDEPRECATED-wed-23/CMSSW_12_2_CMSDEPRECATED_X_2021-11-10-2300

Maybe @smuzaffar can comment on how the CMSDEPRECATED IBs are built

@tvami
Copy link
Contributor

tvami commented Nov 11, 2021

I think the flag is actually -Wdeprecated-declarations

@smuzaffar
Copy link
Contributor

@tvami @battibass , for esConsumes you can either use CMSDEPRECATED IB or for normal IBs, you can set USER_CXXFLAGS=-DUSE_CMS_DEPRECATED environment

@battibass battibass deleted the esConsumes_mu_test branch June 8, 2023 12:05
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.

6 participants