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.

Sign up for GitHub

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

Jump to bottom

[13_0_HLT_X] Improve memory usage in ParameterSet #44921

Merged
merged 12 commits into from
May 22, 2024
Merged

[13_0_HLT_X] Improve memory usage in ParameterSet #44921

merged 12 commits into from
May 22, 2024

Conversation

makortel
Copy link
Contributor

@makortel makortel commented May 7, 2024 •
edited

PR description:

This PR backports #42742 in order to allow a file written in 14_0_X to be read by a 13_0_X cmsRun in order to run the HLT step in 13_0_X in the upcoming 2023 MC campaign.

The last commit was needed to get the backport to compile, because of #43898 was in 13_0_X already before the backport.

Note that files written with a release containing this PR will be unreadable by earlier 13_0_X releases. Therefore this PR is to be merged only in the 13_0_HLT_X branch.

Resolves cms-sw/framework-team#918

PR validation:

I modified the test_MC_23_setup test, added in #44578, to use my local 13_0_18 + this PR developer area and to fail if the HLT step fails, and the test passed.

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:

Backport of #42742

Dr15Jones and others added 12 commits May 6, 2024 23:46
@Dr15Jones @makortel
Simplify encoding of string in edm::ParameterSet
d6a3b31 
- store strings directly with trailing \0 instead of converting
to hex values first.
- replaced cppunit with catch2 test
@Dr15Jones @makortel
Add test for decoding old string encoding
0239def
@Dr15Jones @makortel
Update tests that depend on PSet ID values
9c125a9
@Dr15Jones @makortel
Use std::string_view where appropriate
8ed06e0
@Dr15Jones @makortel
Added an edm::split which tokenizes one at a time
0da2633
@Dr15Jones @makortel
Added tests for existsAs for string and vstring
fd8183e
@Dr15Jones @makortel
Updated getParameter for PdtEntry for new string handling
3df143f
@Dr15Jones @makortel
Fix existAs when dealing with old encodings for string and vstring
62cd4fd
@Dr15Jones @makortel
Test that Event::triggerNames works with old RAW files
577fd69 
Change to PSet string encodings temporarily broke this. This will
now test it works.
@Dr15Jones @makortel
Properly handle old encoding of PSet and vPSet
c07dfde
@Dr15Jones @makortel
Improvements based on code review
d70104e
@makortel
Fix compilation
5916b34 
Caused by backporting cms-sw#42742 on
top of cms-sw#43898
@makortel makortel marked this pull request as ready for review May 7, 2024 14:33
@cmsbuild cmsbuild added this to the CMSSW_13_0_X milestone May 7, 2024
@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2024 •
edited

A new Pull Request was created by @makortel for CMSSW_13_0_X.

It involves the following packages:

  • DQMOffline/Trigger (dqm)
  • FWCore/Framework (core)
  • FWCore/Integration (core)
  • FWCore/ParameterSet (core)
  • IOPool/Common (core)
  • IOPool/Input (core)
  • SimGeneral/HepPDTRecord (simulation)

@mdhildreth, @antoniovagnerini, @smuzaffar, @rvenditti, @syuvivida, @cmsbuild, @nothingface0, @tjavaid, @civanch, @makortel, @Dr15Jones can you please review it and eventually sign? Thanks.
@HuguesBrun, @mtosi, @cericeci, @missirol, @Fedespring, @wddgit, @jhgoh, @rociovilar, @slomeo, @trocino, @fabiocos this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2024 •
edited

cms-bot internal usage

@makortel
Copy link
Contributor Author

makortel commented May 7, 2024

hold

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2024

Pull request has been put on hold by @makortel
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 May 7, 2024
@makortel
Copy link
Contributor Author

makortel commented May 7, 2024

@cms-sw/orp-l2 @cms-sw/pdmv-l2 @cms-sw/ppd-l2 Few weeks back we discussed in ORP that this PR would be merged into a special e.g. 13_0_18_HLT release that would then be used for the HLT step in the upcoming MC production (because files written with this PR would be unreadable with 13_0_X releases without this PR). Just to confirm, is still this the case?

@smuzaffar What kind of additional setup we'd need for that special release?

(I assume everything agreed here will apply similarly to a future 12_4_X backport PR)

@makortel
Copy link
Contributor Author

makortel commented May 7, 2024

@cmsbuild, please test

Let's test it anyhow

@smuzaffar
Copy link
Contributor

smuzaffar commented May 7, 2024 •
edited

@smuzaffar What kind of additional setup we'd need for that special release?

@makortel , I think creating a cms-sw/cmssw CMSSW_13_0_HLT_X branch should be enough take care of 13_0_18_HLT release. This PR should be then merged in to CMSSW_13_0_HLT_X branch. All the changes in CMSSW_13_0_X branch can be automatically forward ported the CMSSW_13_0_HLT_X

By the way, we also have dedicated CMSSW_13_0_HeavyIon_X branch for HI changes

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@makortel
Copy link
Contributor Author

@cmsbuild, please abort

@makortel
Copy link
Contributor Author

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2073d2/39414/summary.html
COMMIT: 5916b34
CMSSW: CMSSW_13_0_HLT_X_2024-05-14-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44921/39414/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 237 lines from the logs
  • Reco comparison results: 15 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3282428
  • DQMHistoTests: Total failures: 983
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3281423
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -308.312 KiB( 48 files compared)
  • DQMHistoSizes: changed ( 1000.0 ): -308.312 KiB HLT/EGM
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@makortel
Copy link
Contributor Author

Comparisons show #39803

@makortel
Copy link
Contributor Author

+core

@makortel
Copy link
Contributor Author

makortel commented May 16, 2024 •
edited

@cms-sw/orp-l2 Please double check the base branch before merging :)

@makortel
Copy link
Contributor Author

makortel commented May 17, 2024 •
edited

@cms-sw/dqm-l2 @cms-sw/simulation-l2 Could you please review and sign this backport?

@makortel
Copy link
Contributor Author

backport of #42742

@civanch
Copy link
Contributor

civanch commented May 17, 2024

+1

fine to sim but test is not complete?

@makortel
Copy link
Contributor Author

The tests on top of 13_0_HLT_X #44921 (comment) succeeded, but maybe the bot still looks for the 13_0_X tests in #44921 (comment) for the test label?

@makortel
Copy link
Contributor Author

@cms-sw/dqm-l2 Could you review and sign? Thanks!

@makortel makortel changed the title [13_0_X] Improve memory usage in ParameterSet [13_0_HLT_X] Improve memory usage in ParameterSet May 21, 2024
@tjavaid
Copy link

tjavaid commented May 22, 2024

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_13_0_HLT_X IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @rappoccio, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

+1

@rappoccio
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit de6346f into cms-sw:CMSSW_13_0_HLT_X May 22, 2024
15 of 16 checks passed
@makortel makortel deleted the newStringParameterSet_130x branch May 22, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
backport-ok core-approved dqm-approved fully-signed orp-approved simulation-approved tests-rejected
Projects
None yet
Milestone
CMSSW_13_0_X
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@makortel @cmsbuild @smuzaffar @AdrianoDee @civanch @tjavaid @rappoccio @Dr15Jones