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

[Backport] [GEM] Unpacker update for the vfat masking information [12_4_X] #39489

Closed
wants to merge 3 commits into from

Conversation

yeckang
Copy link
Contributor

@yeckang yeckang commented Sep 23, 2022

PR description:

  • GEM Unpacker and data formats are updated to save the vfat masking information
  • Minor bugfix for the VFATStatus (Positional information)
  • The results of this update has approved in GEM DPG meeting (link)
  • Backport to CMSSW_12_5_X and CMSSW_12_4_X are needed.

PR validation:

  • The code format has applied with scram build code-format and tested with scram build code-checks.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @yeckang (Yechan Kang) for CMSSW_12_4_X.

It involves the following packages:

  • DQM/GEM (dqm)
  • DataFormats/GEMDigi (upgrade, simulation)
  • EventFilter/GEMRawToDigi (reconstruction)

@civanch, @clacaputo, @mandrenguyen, @emanueleusai, @ahmad3213, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @mdhildreth, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@giovanni-mocellin, @jshlee, @rovere, @eyigitba, @Martin-Grunewald, @missirol, @watson-ij, @trtomei, @beaucero this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

What is the argument for backporting this into 12_4_X?
12_4_X is a closed release that is already being used in data taking.

@emanueleusai
Copy link
Member

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-05078d/27769/summary.html
COMMIT: 7f6296c
CMSSW: CMSSW_12_4_X_2022-09-26-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39489/27769/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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3677402
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3677372
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@emanueleusai
Copy link
Member

emanueleusai commented Sep 26, 2022

What is the argument for backporting this into 12_4_X? 12_4_X is a closed release that is already being used in data taking.

@mandrenguyen we can add one or more PRs to DQM machines at P5 on top of the current release used online in order to fix issues that would otherwise affect DQM operations.
Anything beyond DQM online use probably requires a new patch/release as you mentioned and should be discussed in ORP.

@yeckang can you please confirm you only need this 12_4 backport for deployment to online DQM machines?

Edit: clarification

@clacaputo
Copy link
Contributor

@yeckang can you please confirm you only need this 12_4 backport for deployment to online DQM machines?

Hi @yeckang , could you please clarify this point? Thanks

@yeckang
Copy link
Contributor Author

yeckang commented Sep 27, 2022

@emanueleusai @clacaputo
The affected objects by this PR are only used in DQM. So, this PR is for DQM machines.

@emanueleusai
Copy link
Member

tests at P5 completed successfully, everything looks fine with collision run 357900.
We are approving the DQM part of this PR, but we will not deploy the PR on our machines at P5 until the discussion around the data format/reco part is resolved.
Adding this PR on top of the current release on our machines at P5 should be relatively safe, because the DQM GUI is the end user of our stream, but having a stray PR (i.e. not planned to be part of a future 12_4 release) in our machines should be avoided.

@emanueleusai
Copy link
Member

+1

  • P5 test completed successfully
  • approving the DQM part of this PR

@mandrenguyen
Copy link
Contributor

please close
As agreed in today's reco meeting, this PR will only be deployed in the online DQM in 12_4_X, and will be incorporated into cmssw for >=125X
Of course, if GEM experts need to revisit that decision we can reopen.

@cmsbuild cmsbuild closed this Sep 30, 2022
@perrotta
Copy link
Contributor

Technical question: wouldn't it be preferable to keep it open (maybe on hold) so that GEM and/or DQM can more easily merge it further, and keep it up to date as well, if some newer 12_4_X version will be installed online in the future?

@mandrenguyen
Copy link
Contributor

please open
(not sure that command actually works)
@perrotta you know better than me. For sure we can simply put it on hold for a while.

@cmsbuild cmsbuild reopened this Sep 30, 2022
@mandrenguyen
Copy link
Contributor

please hold
(good bot!)

@perrotta
Copy link
Contributor

please hold (good bot!)

you are too much polite: "hold" alone will work here

@perrotta
Copy link
Contributor

hold

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @perrotta
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@cmsbuild cmsbuild added the hold label Sep 30, 2022
@emanueleusai
Copy link
Member

@perrotta yes it is easier for us to have an actual PR, even if it won't be part of an official 12_4 release. Thanks for bringing this up.

@emanueleusai
Copy link
Member

Given the resolution of the matter at the reco meeting (thanks @mandrenguyen for the update) we will deploy the PR soon to our online machines.

@civanch
Copy link
Contributor

civanch commented Oct 17, 2022

+1

@srimanob
Copy link
Contributor

srimanob commented Dec 5, 2022

If this PR is no need, can we close @yeckang ? Thx.

@perrotta
Copy link
Contributor

As discussed and agreed with DQM and all other groups present at the ORP meeting on Dec 13, we can now close this PR (which will never be merged in the offline release anyhow) and clean up a bit our queues

@perrotta perrotta closed this Dec 13, 2022
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.

9 participants