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

Save repacked raw data in FVT, for express wfs #25178

Merged
merged 1 commit into from Nov 10, 2018

Conversation

mandrenguyen
Copy link
Contributor

The RAW data with label rawDataRepacker is missing from express reco, which should store FVT content.
This adds it. Was tested by re-running express on streamers.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

A new Pull Request was created by @mandrenguyen (Matthew Nguyen) for master.

It involves the following packages:

Configuration/EventContent

@cmsbuild, @franzoni, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald 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

for _entry in [FEVTDEBUGEventContent,FEVTDEBUGHLTEventContent,FEVTEventContent]:
run2_GEM_2017.toModify(_entry, outputCommands = _entry.outputCommands + ['keep *_muonGEMDigis_*_*'])
run3_GEM.toModify(_entry, outputCommands = _entry.outputCommands + ['keep *_muonGEMDigis_*_*'])
phase2_muon.toModify(_entry, outputCommands = _entry.outputCommands + ['keep *_muonGEMDigis_*_*'])

pp_on_AA_2018.toModify(_entry, outputCommands = _entry.outputCommands + ['keep FEDRawDataCollection_rawDataRepacker_*_*'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this need to be handled separately from the rest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidlange6 do you mean not modifying directly the RAW content?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no simple obvious alternative that's clearly not going to break anything outside of pp_on_AA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slava77 the initial simple attempt to modify RAW was not working...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slava77 the initial simple attempt to modify RAW was not working...

I'm not sure if this is connected with David's comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slava77 well, the modification tried was just for pp_on_AA, changing the base definition is different. I do not see how this change might break other things, and I have checked that it solves the observed problem, so in principle it may be merged if a fast patch is needed

@slava77
Copy link
Contributor

slava77 commented Nov 9, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31565/console Started: 2018/11/09 17:57

@fabiocos
Copy link
Contributor

fabiocos commented Nov 9, 2018

Testing express with Configuration/DataProcessing on recent data

python RunExpressProcessing.py --scenario ppEra_Run2_2018_pp_on_AA --global-tag 103X_dataRun2_Express_v2 --lfn /store/t0streamer/Data/HIExpress/000/326/398/run326398_ls0502_streamHIExpress_StorageManager.dat --fevt --dqmio --alcarecos=TkAlMinBias+SiStripCalZeroBias

I get for the fevt output

edmDumpEventContent output.root | grep FED
FEDRawDataCollection "rawDataRepacker" "" "HLT"
edmNew::DetSetVector "siPixelDigis" "" "RECO"
vector "ctppsDiamondRawToDigi" "TimingDiamond" "RECO"
vector "totemRPRawToDigi" "TrackingStrip" "RECO"

with the desired collection as intended for FEVT

@davidlange6
Copy link
Contributor

davidlange6 commented Nov 9, 2018 via email

@slava77
Copy link
Contributor

slava77 commented Nov 9, 2018

Right -as done for the other changes in the pr. We want to protect against something?

I have less doubts about having GEMdigis in the default (without eras): they can be moved to the default.
The HI case with possible varieties of conflicts in the multiple FED collection definition is a stronger concern

@fabiocos
Copy link
Contributor

fabiocos commented Nov 9, 2018

+operations

the fix adds the FEDRawDataCollection to the FEVT event output as requested

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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)

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2018

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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-25178/7.3_CosmicsSPLoose_UP18+CosmicsSPLoose_UP18+DIGICOS_UP18+RECOCOS_UP18+ALCACOS_UP18+HARVESTCOS_UP18

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 3013827
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3013629
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 134 log files, 14 edm output root files, 32 DQM output files

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit a8e8c78 into cms-sw:master Nov 10, 2018
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

5 participants