simHcalTriggerPrimitiveDigis.weightsQIE11 PSet uses number as keyword

[silviodonato]

Minimal code to reproduce the error. Create a file test.py containing

import FWCore.ParameterSet.Config as cms
process = cms.Process("TEST")
process.load('SimCalorimetry.HcalTrigPrimProducers.hcaltpdigi_cfi')
process.p = cms.Path(process.simHcalTriggerPrimitiveDigis)                                                                    

Then

edmConfigDump test.py > dump.py && python dump.py

gives

  File "dump.py", line 105
    1 = cms.untracked.vdouble(1.0, 1.0),
SyntaxError: keyword can't be an expression

The origin of the problem is #31661 which defines weightsQIE11 using dict

    weightsQIE11 = cms.untracked.PSet(**dict([
        ("1",  cms.untracked.vdouble(1.0, 1.0)),
        ("2",  cms.untracked.vdouble(1.0, 1.0)),
      [...]

https://github.com/cms-sw/cmssw/blob/master/SimCalorimetry/HcalTrigPrimProducers/python/hcaltpdigi_cfi.py#L19
@JHiltbrand : please rename the variables from "1", "2", ... to something else!
Eg.

    weightsQIE11 = cms.untracked.PSet(
        var1 =  cms.untracked.vdouble(1.0, 1.0),
        var2 =  cms.untracked.vdouble(1.0, 1.0),
      [...]

@cms-sw/core-l2 : please note that python test.py gives no error, while python dump.py gives error. Do you expect this behaviour?

[cmsbuild]

A new Issue was created by @silviodonato Silvio Donato.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[silviodonato]

assign core, l1

[cmsbuild]

New categories assigned: core,l1

@Dr15Jones,@smuzaffar,@rekovic,@makortel,@jmduarte you have been requested to review this Pull request/Issue and eventually sign? Thanks

[makortel]

please note that python test.py gives no error, while python dump.py gives error. Do you expect this behaviour?

The behavior is not unexpected, the keys of PSet are intended to be valid python identifiers, although when effectively using setattr() "anything" works (until edmConfigDump).

We could think of (somehow) enforcing the keys of PSet to be valid identifiers.

[davidlange6]

I’m naively surprised that python allows this - but ok it does.. One approach in python3 is just to check Import keyword if varname.isidentifier() and not keyword.iskeyword(varname) Python2 could borrow code from here https://stackoverflow.com/questions/12700893/how-to-check-if-a-string-is-a-valid-python-identifier-including-keyword-check

…

On Nov 6, 2020, at 3:32 PM, Matti Kortelainen ***@***.***> wrote: please note that python test.py gives no error, while python dump.py gives error. Do you expect this behaviour? The behavior is not unexpected, the keys of PSet are intended to be valid python identifiers, although when effectively using setattr() "anything" works (until edmConfigDump). We could think of (somehow) enforcing the keys of PSet to be valid identifiers. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[makortel]

I’m naively surprised that python allows this - but ok it does..

Internally they're all dictionaries, and dictionary allows "anything" as keys (e.g. {1: 2}).

Import keyword
if varname.isidentifier() and not keyword.iskeyword(varname)

Thanks, this could be something to look into.

[davidlange6]

On Nov 6, 2020, at 4:20 PM, Matti Kortelainen ***@***.***> wrote: I’m naively surprised that python allows this - but ok it does.. Internally they're all dictionaries, and dictionary allows "anything" as keys (e.g. {1: 2}).

Yes. And what I didn’t think of is that FWCore/ParameterSet never uses **kwargs as variable names and rather just as strings (By chance). So effectively they remain as dictionaries..

…

Import keyword if varname.isidentifier() and not keyword.iskeyword(varname) Thanks, this could be something to look into. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[JHiltbrand]

https://github.com/cms-sw/cmssw/blob/master/SimCalorimetry/HcalTrigPrimProducers/python/hcaltpdigi_cfi.py#L19
@JHiltbrand : please rename the variables from "1", "2", ... to something else!
Eg.

    weightsQIE11 = cms.untracked.PSet(
        var1 =  cms.untracked.vdouble(1.0, 1.0),
        var2 =  cms.untracked.vdouble(1.0, 1.0),
      [...]

Hi @silviodonato ,

I am working on the changes now.

[JHiltbrand]

Hi @silviodonato ,

Please refer to #32067 for addressing this issue.