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

refactor: disable DQM pb files production #46662

Merged

Conversation

gabrielmscampos
Copy link
Contributor

  • Replace the current dqmSaver class with a stub class, thus disabling the pb production everywhere this EDAnalyzer is used.

PR description:

Since we are disabling the anelasticDQM process in the DQM online machines, we are going to start accumulating PB files there. The simplest solution to avoid accumulating files is to disable the PB file production in all clients, because:

  • They were created for the New DQMGUI, that is not deployed (which means we are wasting resources generating those files).
  • It is easier to replace only this file (if not used outside DQM) instead of disabling the PB production in each client.

PR validation:

Tested in DQM Playback environment, all plots behaving as expected and PB files are not created anymore.

The original dqmSaver (PB) EDAnalyzer is called from the DQM clients under DQM/Integration/python/clients (results found using grep -rn "dqmSaverPB" *, the BeamMonitor test and following files in EventFilter (results found using grep -rn "DQMFileSaverPB_cfi" *):

Nevertheless, the original EDAnalyzer seems to be ignored in HLTTrigger confdb, given the following:

Any input on this is appreciated, since later on a flag with the same name seems to be written in the auto generated file:

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 11, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46662/42585

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DQMServices/FileIO (dqm)

@antoniovagnerini, @cmsbuild, @rseidita can you please review it and eventually sign? Thanks.
@barvic this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@gabrielmscampos
Copy link
Contributor Author

FYI @mmusich

@mmusich
Copy link
Contributor

mmusich commented Nov 11, 2024

@cms-sw/hlt-l2 @fwyzard FYI

@mmusich
Copy link
Contributor

mmusich commented Nov 11, 2024

concerning:

Nevertheless, the original EDAnalyzer seems to be ignored in HLTTrigger confdb, given the following:
HLTrigger/Configuration/python/Tools/confdb.py#L701

it is ignored in the offline dumps of the HLT menu, but we definitely use it the online HLT menu and keep wanting to do so.

@mmusich
Copy link
Contributor

mmusich commented Nov 11, 2024

It is easier to replace only this file (if not used outside DQM) instead of disabling the PB production in each client.

why?

@Martin-Grunewald
Copy link
Contributor

Martin-Grunewald commented Nov 11, 2024

Nevertheless, the original EDAnalyzer seems to be ignored in HLTTrigger confdb, given the following:

https://github.com/cms-sw/cmssw/blob/master/HLTrigger/Configuration/python/Tools/confdb.py#L701

Any input on this is appreciated, since later on a flag with the same name seems to be written in the auto generated file:

https://github.com/cms-sw/cmssw/blob/master/HLTrigger/Configuration/python/Tools/confdb.py#L814

Line 814 simple removes the module instance from the dump for offline!

@gabrielmscampos
Copy link
Contributor Author

It is easier to replace only this file (if not used outside DQM) instead of disabling the PB production in each client.

why?

  • We are modifying a single line in the DQMFileSaverPB_cfi instead of 46 files in DQM/Integration/python/clients/ + DQM/BeamMonitor/test/Online_BeamMonitor_file.py.
  • It will be easier to revert when the PB files are actually needed in the future.

However, since the online HLT menu is using it. I guess the only solution is modifying all the files.

# by replacing the dqmSaver with a stub dqmSaver.
# dqmSaver = cms.EDAnalyzer("DQMFileSaverPB",

dqmSaver = cms.EDAnalyzer("DQMFileSaverPBStub",
Copy link
Contributor

@mmusich mmusich Nov 11, 2024

Choose a reason for hiding this comment

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

@Martin-Grunewald is this going to mess up with the parsing of the confDB GUI?

Copy link
Contributor

@mmusich mmusich Nov 11, 2024

Choose a reason for hiding this comment

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

shall we make this

descriptions.addDefault(desc);

actually write out a cfi with addWithDefaultLabel?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would need a proper fillDescriptions method with all the parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the end these are two different plugins and thus need two separate cfi files (as each cfi file contains the class name).

# by replacing the dqmSaver with a stub dqmSaver.
# dqmSaver = cms.EDAnalyzer("DQMFileSaverPB",

dqmSaver = cms.EDAnalyzer("DQMFileSaverPBStub",
Copy link
Contributor

@Martin-Grunewald Martin-Grunewald Nov 11, 2024

Choose a reason for hiding this comment

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

This would remove the original DQMFileSaverPB from being seen by ConfDB parsing (no cfi file with it!) and thus no longer available for HLT menus online or offline. As Marco mentioned we want to use the plugin for online, so we'd need another solution here, for example, a new separate cfi file DQMFileSaverPBStub_cfi.py, and not removing the old one.

@mmusich
Copy link
Contributor

mmusich commented Nov 11, 2024

However, since the online HLT menu is using it. I guess the only solution is modifying all the files.

I don't think this PR per se is going to be harmful to the HLT online, provided you address #46662 (comment)..

@gabrielmscampos
Copy link
Contributor Author

However, since the online HLT menu is using it. I guess the only solution is modifying all the files.

I don't think this PR per se is going to be harmful to the HLT online, provided you address #46662 (comment)..

Got it, I can create a new EDAnalyzer for the DQM clients in order to preserve the current one for HLT.

Thanks!

@mmusich
Copy link
Contributor

mmusich commented Nov 11, 2024

We are modifying a single line in the DQMFileSaverPB_cfi instead of 46 files in DQM/Integration/python/clients/ + DQM/BeamMonitor/test/Online_BeamMonitor_file.py.

maybe I am missing something trivial, but where is it referenced in all of these files?
It looks to me that all the inheritance comes from things like

process.load("DQM.Integration.config.environment_cfi")

via

from DQMServices.FileIO.DQMFileSaverPB_cfi import dqmSaver as dqmSaverPB

If you want to just change the behavior in the online clients can't you create a new cfi file for DQMFileSaverPBStub and replace it in the environment configuration fragment?

something like:

 from DQMServices.FileIO.DQMFileSaverPBStub_cfi import dqmSaver as dqmSaverPB 

@gabrielmscampos
Copy link
Contributor Author

We are modifying a single line in the DQMFileSaverPB_cfi instead of 46 files in DQM/Integration/python/clients/ + DQM/BeamMonitor/test/Online_BeamMonitor_file.py.

maybe I am missing something trivial, but where is it referenced in all of these files? It looks to me that all the inheritance comes from things like

process.load("DQM.Integration.config.environment_cfi")

via

from DQMServices.FileIO.DQMFileSaverPB_cfi import dqmSaver as dqmSaverPB

If you want to just change the behavior in the online clients can't you create a new cfi file for DQMFileSaverPBStub and replace it in the environment configuration fragment?

something like:

 from DQMServices.FileIO.DQMFileSaverPBStub_cfi import dqmSaver as dqmSaverPB 

Yes, that is what I'm going to do. Initially I thought in removing the references to dqmSaverPB in all clients, that is why I mentioned all the files. However, I'll update only the environment_cfi and cross-check this is not used elsewhere.

@fwyzard
Copy link
Contributor

fwyzard commented Nov 12, 2024

hold

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0cf9c0/42835/summary.html
COMMIT: a32c7d3
CMSSW: CMSSW_14_2_X_2024-11-13-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46662/42835/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 2 errors in the following unit tests:

---> test TestDQMServicesDemo had ERRORS
---> test TestDQMGUIUpload had ERRORS

Comparison Summary

Summary:

@nothingface0
Copy link
Contributor

@smuzaffar The TestDQMServicesDemo seems to fail due to a missing (automatically generated?) cfi file:

----- Begin Fatal Exception 14-Nov-2024 10:49:09 CET-----------------------
An exception of category 'ConfigFileReadError' occurred while
   [0] Processing the python configuration file named /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_2_X_2024-11-13-2300/src/DQMServices/Demo/test/run_analyzers_cfg.py
Exception Message:
 unknown python problem occurred.
ModuleNotFoundError: No module named 'DQMServices.Demo.test_cfi'

At:
  /cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02863/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_14_2_X_2024-11-13-2300/src/FWCore/ParameterSet/python/Config.py(762): load
  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_14_2_X_2024-11-13-2300/src/DQMServices/Demo/test/run_analyzers_cfg.py(7): <module>

----- End Fatal Exception -------------------------------------------------

Are we missing something?

@smuzaffar
Copy link
Contributor

please test

looks like latest bot change cms-sw/cms-bot#2361 broke the PR testing

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests
Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0cf9c0/42846/summary.html
COMMIT: a32c7d3
CMSSW: CMSSW_14_2_X_2024-11-13-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46662/42846/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 1 errors in the following unit tests:

---> test TestDQMGUIUpload had ERRORS

Comparison Summary

Summary:

@antoniovagnerini
Copy link

please test

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0cf9c0/42921/summary.html
COMMIT: a32c7d3
CMSSW: CMSSW_14_2_X_2024-11-17-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46662/42921/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@antoniovagnerini
Copy link

Spurious errors in DQM bin-by-bin comparison in Tracker Phase 2 WS seem to have been introduced by #46717, which was merged 5 h ago , see https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_14_2_X_2024-11-17-0000+6d35e2/65692/29634.0_TTbar_14TeV+Run4D110/

@antoniovagnerini
Copy link

+1

@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. @rappoccio, @mandrenguyen, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 8fa55df into cms-sw:master Nov 18, 2024
11 checks passed
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.

10 participants