SONIC configuration updates #47748
SONIC configuration updates #47748
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47748/44330
|
|
A new Pull Request was created by @kpedro88 for master. It involves the following packages:
@AdrianoDee, @Moanwar, @cmsbuild, @DickyChant, @fwyzard, @makortel, @miquork, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters: |
|
please test |
| if not hasattr(process,'MessageLogger'): | ||
| process.load('FWCore/MessageService/MessageLogger_cfi') |
There was a problem hiding this comment.
Just to note that this piece is needed only if the user has explicitly deleted process.MessageLogger. MessageLogger has been part of cms.Process for quite some time already.
If kept, while this works, I'd prefer
| if not hasattr(process,'MessageLogger'): | |
| process.load('FWCore/MessageService/MessageLogger_cfi') | |
| if not hasattr(process,'MessageLogger'): | |
| process.load('FWCore.MessageService.MessageLogger_cfi') |
There was a problem hiding this comment.
Ah, I actually did not know it was included in the initialization of cms.Process. Then I think this condition is not necessary.
| setattr(process.MessageLogger.cerr, msg, | ||
| cms.untracked.PSet( | ||
| limit = cms.untracked.int32(10000000), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Could be
| setattr(process.MessageLogger.cerr, msg, | |
| cms.untracked.PSet( | |
| limit = cms.untracked.int32(10000000), | |
| ) | |
| ) | |
| setattr(process.MessageLogger.cerr, msg, | |
| dict( | |
| limit = 10000000, | |
| ) | |
| ) |
| cms.PSet( | ||
| name = cms.untracked.string(options.serverName), | ||
| address = cms.untracked.string(options.address), | ||
| port = cms.untracked.uint32(options.port), | ||
| useSsl = cms.untracked.bool(options.ssl), | ||
| rootCertificates = cms.untracked.string(""), | ||
| privateKey = cms.untracked.string(""), | ||
| certificateChain = cms.untracked.string(""), | ||
| ) |
There was a problem hiding this comment.
Could be
| cms.PSet( | |
| name = cms.untracked.string(options.serverName), | |
| address = cms.untracked.string(options.address), | |
| port = cms.untracked.uint32(options.port), | |
| useSsl = cms.untracked.bool(options.ssl), | |
| rootCertificates = cms.untracked.string(""), | |
| privateKey = cms.untracked.string(""), | |
| certificateChain = cms.untracked.string(""), | |
| ) | |
| dict( | |
| name = options.serverName, | |
| address = options.address, | |
| port = options.port, | |
| useSsl = options.ssl, | |
| ) |
| return dict( | ||
| compression = cms.untracked.string(options.compression), | ||
| useSharedMemory = cms.untracked.bool(not options.noShm), | ||
| timeout = cms.untracked.uint32(options.timeout), | ||
| timeoutUnit = cms.untracked.string(options.timeoutUnit), | ||
| allowedTries = cms.untracked.uint32(options.tries), | ||
| ) |
There was a problem hiding this comment.
Could probably be
| return dict( | |
| compression = cms.untracked.string(options.compression), | |
| useSharedMemory = cms.untracked.bool(not options.noShm), | |
| timeout = cms.untracked.uint32(options.timeout), | |
| timeoutUnit = cms.untracked.string(options.timeoutUnit), | |
| allowedTries = cms.untracked.uint32(options.tries), | |
| ) | |
| return dict( | |
| compression = options.compression, | |
| useSharedMemory = not options.noShm, | |
| timeout = options.timeout, | |
| timeoutUnit = options.timeoutUnit, | |
| allowedTries = options.tries, | |
| ) |
if the EDModule object being modified is expected to originate from fillDescriptions().
There was a problem hiding this comment.
Unfortunately, that is not always the case. I had originally written it this way and then hit the "parameter does not already exist" error. I do not know another way around that besides always specifying the types.
| modules.update(process._Process__producers) | ||
| modules.update(process._Process__filters) | ||
| modules.update(process._Process__analyzers) |
There was a problem hiding this comment.
Please use e.g.
| modules.update(process._Process__producers) | |
| modules.update(process._Process__filters) | |
| modules.update(process._Process__analyzers) | |
| modules.update(process.producers_()) | |
| modules.update(process.filters_()) | |
| modules.update(process.analyzers_()) |
instead of accessing "private" members of the Process.
| def applyClientOptions(client, options): | ||
| return configureClient(client, **getClientOptions(options)) | ||
|
|
||
| def configureModules(process, modules=None, returnConfigured=False, **kwargs): |
There was a problem hiding this comment.
Just to make sure, if user specifies the modules argument, they are expected to provide it as a dictionary of those along
labels = # list of strings
modules = {label: getattr(process, label) for label in labels}?
There was a problem hiding this comment.
Yes, this is mainly there to allow users to apply a configuration to a selected subset of modules (based on whatever criteria they might have). e.g. they could populate the dictionary by filtering out unwanted entries from process.producers_().
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-started
code-checks-approved
labels
|
@makortel implemented all but #47748 (comment) (as explained in comment there) |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47748/44332
|
|
Pull request #47748 was updated. @AdrianoDee, @Moanwar, @cmsbuild, @DickyChant, @fwyzard, @makortel, @miquork, @srimanob, @subirsarkar can you please check and sign again. |
|
please test |
|
+1 Size: This PR adds an extra 52KB to repository Comparison SummarySummary:
|
|
Comparison differences are related to #47071 |
|
+heterogeneous |
|
@cms-sw/upgrade-l2 @cms-sw/pdmv-l2 please check and sign (just adding some Run 3 special workflows) |
|
+Upgrade |
|
+pdmv |
cmsbuild
added
fully-signed
pdmv-approved
and removed
pending-signatures
pdmv-pending
labels
|
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. @antoniovilela, @sextonkennedy, @mandrenguyen, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR makes some improvements to configuration details for SONIC:
PR validation:
Ran unit tests and matrix workflows successfully with expected output.
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:
Not a backport, not intended to be backported.