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

Subject main tau ID payloads to global tag control #28063

Merged

Conversation

swozniewski
Copy link
Contributor

PR description:

Payload labels are changed such that payloads for current anti-electron MVA ID and the three MVA tau IDs DBoldDM, DBnewDM, DBoldDMdR03 are loaded via global tag.
This requires global tag configurations as in test global tag:

110X_upgrade2018_realistic_Candidate_2019_09_22_09_29_04

Deprecated PW MVA tau IDs are removed from the reco and pat workflows.
In RECO (HPSPFTaus_cff.py), no payloads are loaded directly from DB anymore.
In NanoAOD, the upper IDs are loaded via GT as well, but further IDs which shall still be producible for experimental and comparison purposes, or if a very new payload is not yet available via GT, can still be loaded directly from DB as before.

PR validation:

RECO+PAT step were run with test GT and outputs compared, same for NanoAOD
Matrix tests fail due to wrong GT

if this PR is a backport please specify the original PR:

no backport planned

@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-28063/12003

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @swozniewski for master.

It involves the following packages:

PhysicsTools/NanoAOD
PhysicsTools/PatAlgos
RecoTauTag/Configuration

@perrotta, @cmsbuild, @fgolf, @slava77, @santocch, @peruzzim can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @emilbols, @HeinerTholen, @peruzzim, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @acaudron, @jdolen, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @andrzejnovak, @clelange, @riga, @JyothsnaKomaragiri, @mverzett, @gpetruc, @mariadalfonso, @pvmulder 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

@perrotta
Copy link
Contributor

assign alca

@cmsbuild
Copy link
Contributor

New categories assigned: alca

@christopheralanwest,@franzoni,@tlampen,@pohsun,@tocheng you have been requested to review this Pull request/Issue and eventually sign? Thanks

@perrotta
Copy link
Contributor

To test this appropriately we need to have the corresponding GT. What are the plans for having it available in the release?
If the one you suggest is just adding payloads, and not touching the existing ones, then it could probably be just replaced in autocond and the tests run with it.
Otherwise, a PR (even "dummy") that replaces such a GT should be prepared in order to run it together with this one for the tests.
Please decide together with alca which option is most suitable for testing this PR.

@tocheng
Copy link
Contributor

tocheng commented Sep 26, 2019

To test this appropriately we need to have the corresponding GT. What are the plans for having it available in the release?
If the one you suggest is just adding payloads, and not touching the existing ones, then it could probably be just replaced in autocond and the tests run with it.
Otherwise, a PR (even "dummy") that replaces such a GT should be prepared in order to run it together with this one for the tests.
Please decide together with alca which option is most suitable for testing this PR.

I would suggest customizing GT by adding the tags (conditions) in the config files.
Once the tags are tested and this PR is merged, we could add these tags to the GTs in autoCond.

@swozniewski
Copy link
Contributor Author

So we need a PR from the test GT if I understand correctly. @aknayak could you please take care of this?

@peruzzim
Copy link
Contributor

I can see some issues potentially coming from this, they can be handled but it will have to be orchestrated well.
In NANOAOD we use e.g. the GT to read the JEC, and we want to support running on several old datasets for which we need dedicated JEC sets (e.g. one for 2016, one for 2017 etc.).
Therefore one should envisage having separate 110X_... tags, all containing the same tau payloads but different JEC, as we had in the past for 102X central productions.
Before this, we could for the moment (I think) continue using the 102X GT also in 11_0_X, because we basically use it only for JME corrections.
After merging this, if we call the producer without the payload being in the tag it will crash, right?

@swozniewski
Copy link
Contributor Author

Right, with current official GTs CMSSW will complain that the used tags are not available.
As far as I know it is intended to add the tau payloads to several GTs as you suggest, in particular for this reason. @aknayak and @christopheralanwest had an email thread about this and figured out which GT queues to update if I remember correctly.

@peruzzim
Copy link
Contributor

please include XPOG in that thread, there are several combinations of MINIAOD production version + NANOAOD code to support, and we should be sure we don't break any workflow we want to support

@perrotta
Copy link
Contributor

perrotta commented Oct 7, 2019

@swozniewski is there any news about a possible GT to test this?

@tocheng
Copy link
Contributor

tocheng commented Oct 7, 2019

@perrotta AlCaDB is working on it. Expect to have a PR tomorrow, then this PR can be tested.

@tocheng tocheng mentioned this pull request Oct 9, 2019
@tocheng
Copy link
Contributor

tocheng commented Oct 9, 2019

@swozniewski @perrotta PR #28134 is submitted and test is started. If no issue, this PR can be test with PR #28134

@perrotta
Copy link
Contributor

perrotta commented Oct 9, 2019

please test with #28134

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 30, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/3251/console Started: 2019/10/30 14:16

@cmsbuild
Copy link
Contributor

@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-f1dd33/3251/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 2443 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2961036
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2960694
  • 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

@fabiocos
Copy link
Contributor

fabiocos commented Nov 3, 2019

@peruzzim so what is finally the conclusion?

@peruzzim
Copy link
Contributor

peruzzim commented Nov 4, 2019

+xpog

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 4, 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 Nov 4, 2019

+1

@cmsbuild cmsbuild merged commit 0a43aa7 into cms-sw:master Nov 4, 2019
@swozniewski swozniewski deleted the CMSSW_11_0_X_tau-pog_globalTag_slim branch June 19, 2020 13:21
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

9 participants