VarParsing: prevent TypeError for default of int/float/bool list #45640

IzaakWN · 2024-08-05T16:05:31Z

PR description:

This PR patches the handling of defaults in FWCore.ParameterSet.VarParsing to prevent a TypeError for the case of an int/float/bool type in combination with VarParsing.multiplicity.list, where the default is a single int/float/bool value. This bug was described in #44630:

Note:

In order to prevent interfering with any workflows that might rely on the behavior of "list of list" defaults, this PR only prevents the TypeError with an if statement:
https://github.com/IzaakWN/cmssw/blob/dbd4367723a9bcc29fcf308f2902896c2aabbc6e/FWCore/ParameterSet/python/VarParsing.py#L413-L418
This means that if a user wants to use as default a list of multiple int/float/bool values, they will still get a list of lists of those values, same as for strings... E.g., [0,1] → [[0,1]].

opts = VarParsing('standard')

opts.register('myInts0`,  0,  VarParsing.multiplicity.list, VarParsing.varType.int) 
# before this PR: causes TypeError because len(int)
# after this PR: yields list of (single) integer [0] as default

opts.register('myInts1`, [0],  VarParsing.multiplicity.list, VarParsing.varType.int)
# works because len(list)
# yields list of list [[0]] as default

opts.register('myInts2', [0,1],  VarParsing.multiplicity.list, VarParsing.varType.int)
# works because len(list)
# yields list of list [[0,1]] as default

Tagging: @makortel

PR validation:

I setup CMSSW as follows:

source $VO_CMS_SW_DIR/cmsset_default.sh
export SCRAM_ARCH="el9_amd64_gcc12"
CMSSW="CMSSW_14_1_0_pre6"
cmsrel $CMSSW -n ${CMSSW}_varpars
cd ${CMSSW}_varpars/src && cmsenv
git cms-addpkg FWCore/ParameterSet

To test the behavior, I used this reproducible snippet in the python3 command line:

import sys
from FWCore.ParameterSet.VarParsing import VarParsing

def parse(argv):
  print(f">>> Parsing {argv!r}...")
  sys.argv = ['test.py','maxEvents=100']+argv
  opts = VarParsing('standard')
  print(">>> Register...")
  def add(*args,**kwargs):
    try:
      print(f">>>   {args!r}, {kwargs!r}...")
      opts.register(*args,**kwargs)
    except Exception as err:
      print(f">>>   => {err!r}")
  add('myInts0',  '',      VarParsing.multiplicity.list, VarParsing.varType.int) # works because len(str)
  add('myInts1',  0,       VarParsing.multiplicity.list, VarParsing.varType.int) # ERROR because len(int)
  add('myInts2',  [0],     VarParsing.multiplicity.list, VarParsing.varType.int) # works because len(list)
  add('myBools0', '',      VarParsing.multiplicity.list, VarParsing.varType.bool) # works because len(str)
  add('myBools1', True,    VarParsing.multiplicity.list, VarParsing.varType.bool) # ERROR because len(bool)
  add('myBools2', [True],  VarParsing.multiplicity.list, VarParsing.varType.bool) # works because len(list)
  add('myStrs0',  '',      VarParsing.multiplicity.list, VarParsing.varType.string) # works because len(str)
  add('myStrs1',  'foo',   VarParsing.multiplicity.list, VarParsing.varType.string) # works because len(str)
  add('myStrs2',  ['foo'], VarParsing.multiplicity.list, VarParsing.varType.string) # works because len(list)
  opts.parseArguments()
  print(f">>> Parsed:")
  for var in opts._lists:
    if var.startswith('my'):
      print(f">>>   {var:6s} = {getattr(opts,var)!r}")

parse([ ])
parse(['myInts1=0,1','myBools1=True,False','myStrs1=foo,bar','myStrs2=foo,bar'])

Before the changes, there are TypeErrors:

>>> parse([ ])
>>> Parsing []...
>>> Register...
>>>   ('myInts0', '', 'multiplicity_list', '_int'), {}...
>>>   ('myInts1', 0, 'multiplicity_list', '_int'), {}...
>>>   => TypeError("object of type 'int' has no len()")
>>>   ('myInts2', [0], 'multiplicity_list', '_int'), {}...
>>>   ('myBools0', '', 'multiplicity_list', '_bool'), {}...
>>>   ('myBools1', True, 'multiplicity_list', '_bool'), {}...
>>>   => TypeError("object of type 'bool' has no len()")
>>>   ('myBools2', [True], 'multiplicity_list', '_bool'), {}...
>>>   ('myStrs0', '', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs1', 'foo', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs2', ['foo'], 'multiplicity_list', '_string'), {}...
>>> Parsed:
>>>   myInts0 = []
>>>   myInts1 = []
>>>   myInts2 = [[0]]
>>>   myBools0 = []
>>>   myBools1 = []
>>>   myBools2 = [[True]]
>>>   myStrs0 = []
>>>   myStrs1 = ['foo']
>>>   myStrs2 = [['foo']]
>>> parse(['myInts1=0,1','myBools1=True,False','myStrs1=foo,bar','myStrs2=foo,bar'])
>>> Parsing ['myInts1=0,1', 'myBools1=True,False', 'myStrs1=foo,bar', 'myStrs2=foo,bar']...
>>> Register...
>>>   ('myInts0', '', 'multiplicity_list', '_int'), {}...
>>>   ('myInts1', 0, 'multiplicity_list', '_int'), {}...
>>>   => TypeError("object of type 'int' has no len()")
>>>   ('myInts2', [0], 'multiplicity_list', '_int'), {}...
>>>   ('myBools0', '', 'multiplicity_list', '_bool'), {}...
>>>   ('myBools1', True, 'multiplicity_list', '_bool'), {}...
>>>   => TypeError("object of type 'bool' has no len()")
>>>   ('myBools2', [True], 'multiplicity_list', '_bool'), {}...
>>>   ('myStrs0', '', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs1', 'foo', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs2', ['foo'], 'multiplicity_list', '_string'), {}...
>>> Parsed:
>>>   myInts0 = []
>>>   myInts1 = [0, 1]
>>>   myInts2 = [[0]]
>>>   myBools0 = []
>>>   myBools1 = [True, False]
>>>   myBools2 = [[True]]
>>>   myStrs0 = []
>>>   myStrs1 = ['foo', 'bar']
>>>   myStrs2 = ['foo', 'bar']

After the changes, the TypeErrors are gone, and the single defaults are stored as a list of int/float/bool:

>>> parse([ ])
>>> Parsing []...
>>> Register...
>>>   ('myInts0', '', 'multiplicity_list', '_int'), {}...
>>>   ('myInts1', 0, 'multiplicity_list', '_int'), {}...
>>>   ('myInts2', [0], 'multiplicity_list', '_int'), {}...
>>>   ('myBools0', '', 'multiplicity_list', '_bool'), {}...
>>>   ('myBools1', True, 'multiplicity_list', '_bool'), {}...
>>>   ('myBools2', [True], 'multiplicity_list', '_bool'), {}...
>>>   ('myStrs0', '', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs1', 'foo', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs2', ['foo'], 'multiplicity_list', '_string'), {}...
>>> Parsed:
>>>   myInts0 = []
>>>   myInts1 = [0]
>>>   myInts2 = [[0]]
>>>   myBools0 = []
>>>   myBools1 = [True]
>>>   myBools2 = [[True]]
>>>   myStrs0 = []
>>>   myStrs1 = ['foo']
>>>   myStrs2 = [['foo']]
>>> parse(['myInts1=0,1','myBools1=True,False','myStrs1=foo,bar','myStrs2=foo,bar'])
>>> Parsing ['myInts1=0,1', 'myBools1=True,False', 'myStrs1=foo,bar', 'myStrs2=foo,bar']...
>>> Register...
>>>   ('myInts0', '', 'multiplicity_list', '_int'), {}...
>>>   ('myInts1', 0, 'multiplicity_list', '_int'), {}...
>>>   ('myInts2', [0], 'multiplicity_list', '_int'), {}...
>>>   ('myBools0', '', 'multiplicity_list', '_bool'), {}...
>>>   ('myBools1', True, 'multiplicity_list', '_bool'), {}...
>>>   ('myBools2', [True], 'multiplicity_list', '_bool'), {}...
>>>   ('myStrs0', '', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs1', 'foo', 'multiplicity_list', '_string'), {}...
>>>   ('myStrs2', ['foo'], 'multiplicity_list', '_string'), {}...
>>> Parsed:
>>>   myInts0 = []
>>>   myInts1 = [0, 1]
>>>   myInts2 = [[0]]
>>>   myBools0 = []
>>>   myBools1 = [True, False]
>>>   myBools2 = [[True]]
>>>   myStrs0 = []
>>>   myStrs1 = ['foo', 'bar']
>>>   myStrs2 = ['foo', 'bar']

PR tests

Following https://cms-sw.github.io/PRWorkflow.html, I did:

scram b distclean 
git cms-checkdeps -a -A # adds 17 other packages..
scram b -j 8
scram b runtests

As VarParsing is used by many python configuration scripts, git cms-checkdeps -a -A will add 17 additional packages. Not all test are successful, but I checked the same tests fail in a CMSSW project without any changes. As far as I can tell the failures do not come from my side. I assume no workflow contains an exception for the TypeError above.

cmsbuild · 2024-08-05T16:05:56Z

cms-bot internal usage

cmsbuild · 2024-08-05T16:08:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45640/41153

cmsbuild · 2024-08-05T16:08:35Z

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

It involves the following packages:

FWCore/ParameterSet (core)

@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@makortel, @missirol, @wddgit 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

makortel · 2024-08-05T16:54:44Z

Thanks @IzaakWN! Would you be able to add your test snippet as a unit test? E.g. something along

Place the code in e.g. FWCore/ParameterSet/test/test_varparsing_list.py
Add <test name="TestFWCoreParameterSetVarParsingList" command="python3 ${LOCALTOP}/src/FWCore/ParameterSet/test/test_varparsing_list.py"/>

should do the job. You can run the test locally with scram b runtests_TestFWCoreParameterSetVarParsingList.

cmsbuild · 2024-08-07T21:38:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45640/41207

cmsbuild · 2024-08-07T21:38:37Z

Pull request #45640 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again.

makortel · 2024-08-07T21:40:44Z

@cmsbuild, please test

cmsbuild · 2024-08-08T01:19:57Z

-1

Failed Tests: UnitTests
Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-85a10a/40825/summary.html
COMMIT: 5089cc1
CMSSW: CMSSW_14_1_X_2024-08-07-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45640/40825/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 testCondToolsSiStripBuildersReaders had ERRORS

Comparison Summary

Summary:

You potentially added 1 lines to the logs
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 45
DQMHistoTests: Total histograms compared: 3423977
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3423957
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 44 files compared)
Checked 196 log files, 165 edm output root files, 45 DQM output files
TriggerResults: no differences found

IzaakWN · 2024-08-08T06:15:02Z

Looks like the failed jobs comes from the case where the default list for ints is an empty list:

cmssw/CondTools/SiStrip/test/SiStripBadChannelPatcher_cfg.py

Lines 20 to 42 in e0cb38f

    
           options.register ('FEDsToAdd', 
        
                             [], 
        
                             VarParsing.VarParsing.multiplicity.list,  # singleton or list 
        
                             VarParsing.VarParsing.varType.int,        # string, int, or float 
        
                             "list of FEDs to Add") 
        
           options.register ('FEDsToRemove', 
        
                             [], 
        
                             VarParsing.VarParsing.multiplicity.list,  # singleton or list 
        
                             VarParsing.VarParsing.varType.int,        # string, int, or float 
        
                             "list of FEDs to Remove") 
        
           options.register ('DetIdsToAdd', 
        
                             [], 
        
                             VarParsing.VarParsing.multiplicity.list,  # singleton or list 
        
                             VarParsing.VarParsing.varType.int,        # string, int, or float 
        
                             "list of DetIds to Add") 
        
           options.register ('DetIdsToRemove', 
        
                             [], 
        
                             VarParsing.VarParsing.multiplicity.list,  # singleton or list 
        
                             VarParsing.VarParsing.varType.int,        # string, int, or float 
        
                             "list of DetIds to Remove")

which passes the new first term in the condition, but not the old second one...

cmssw/FWCore/ParameterSet/python/VarParsing.py

Lines 416 to 418 in 5089cc1

    
           if (mytype in [VarParsing.varType.bool, VarParsing.varType.int, VarParsing.varType.float] and \ 
        
              default != "") or len (default): # check type to prevent TypeError for bool/int/float defaults 
        
               self._lists[name].append (default) # known bug: default can be list

I can change the condition to ignore empty lists? Something like

if (mytype in [VarParsing.varType.bool, VarParsing.varType.int, VarParsing.varType.float] and \
   default not in ["", []]) or len (default): # check type to prevent TypeError for bool/int/float defaults
    self._lists[name].append (default) # known bug: default can be list

….parseArguments to unit test

IzaakWN · 2024-11-22T13:54:50Z

I find it unfortunate the singular default was allowed at the time (and is used for strings). But, given where we are, I continue to be ok with adding the singular defaults for int/float/bool. But I'm now leaning towards fixing the use case 1 you mentioned in #44630, even if it might break some user code (the L1ConfigWriteSinglePayload_cfg.py in CMSSW itself can and should be fixed).

@IzaakWN Would you be willing to update this PR along this way?

@makortel, to be honest I prefer not to make any changes that will break an unkown large number of scripts, and unit tests that I don't want to be responsible for breaking or fixing. Like you point out, many users implemented workarounds that rely on this behavior, even if it's inconsistent between default and command line (use case (1) in #44630).

I propose to merge this PR with the current fix that only prevents a TypeError:

cmssw/FWCore/ParameterSet/python/VarParsing.py

Lines 416 to 418 in db50eed

    
           if (mytype in [VarParsing.varType.bool, VarParsing.varType.int, VarParsing.varType.float] and \ 
        
              default not in ["",[]]) or len (default): # check type to prevent TypeError for bool/int/float defaults 
        
               self._lists[name].append (default) # known bug: default can be list

Or, if you prefer, the more general solution with hasattr(default,'__len__'):

if (mytype in [VarParsing.varType.bool, VarParsing.varType.int, VarParsing.varType.float] and \
   not hasattr(default,'__len__')) or len (default): # check type to prevent TypeError for bool/int/float defaults
    self._lists[name].append (default) # known bug: default can be list

makortel · 2024-11-27T21:07:30Z

to be honest I prefer not to make any changes that will break an unkown large number of scripts, and unit tests that I don't want to be responsible for breaking or fixing. Like you point out, many users implemented workarounds that rely on this behavior, even if it's inconsistent between default and command line (use case (1) in #44630).

Fair enough. Then the cases that do not work with the "list of lists" I listed in #45640 (comment) should, in principle, be fixed, but I'll open separate issues for them.

I propose to merge this PR with the current fix that only prevents a TypeError:

At this stage let's proceed with minimal effort and continue with this PR as it is. I'll launch last round of tests.

makortel · 2024-11-27T21:08:45Z

@cmsbuild, please test

makortel · 2024-11-27T21:18:24Z

Then there is

cmssw/CalibTracker/SiStripCommon/test/testProduceCalibrationTree_cfg.py

Lines 44 to 46 in ff2f075

options.register('inputFiles',

filesRelValTTbarPileUpGENSIMRECO,

VarParsing.VarParsing.multiplicity.list,

that uses a cms.vstring() (!) as the default value, leading to a list of vstring (of one element), and I'd expect its use later in the configuration to fail.

Playing around it seems I was wrong, this case appears to work (at least with some definition of "work").

cmsbuild · 2024-11-27T22:55:42Z

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-85a10a/43122/summary.html
COMMIT: db50eed
CMSSW: CMSSW_15_0_X_2024-11-27-1400/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45640/43122/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 1 lines from the logs
Reco comparison results: 26 differences found in the comparisons
DQMHistoTests: Total files compared: 46
DQMHistoTests: Total histograms compared: 3483722
DQMHistoTests: Total failures: 1516
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3482186
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
Checked 202 log files, 172 edm output root files, 46 DQM output files
TriggerResults: no differences found

makortel · 2024-11-27T22:59:02Z

Comparison differences are related to #39803 and #43415

makortel · 2024-11-27T23:00:16Z

This PR looks ok then. I'll leave signing it to early next week though in order to be around in case something would go bad in IBs (even if the tests here should have included all packages that depend on VarParsing).

makortel · 2024-12-02T21:26:53Z

+core

cmsbuild · 2024-12-02T21:27:15Z

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

mandrenguyen · 2024-12-03T10:16:17Z

+1

VarParsing: prevent TypeError for default of int/float/bool list

dbd4367

cmsbuild added this to the CMSSW_14_1_X milestone Aug 5, 2024

cmsbuild added core-pending pending-signatures tests-pending orp-pending code-checks-pending labels Aug 5, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 5, 2024

add unit test for VarParsing list default

5089cc1

cmsbuild added code-checks-pending and removed code-checks-approved labels Aug 7, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 7, 2024

cmsbuild added tests-started and removed tests-pending labels Aug 7, 2024

cmsbuild added tests-rejected and removed tests-started labels Aug 8, 2024

VarParsing: skip empty list for int/float/bool list default; add opts…

db50eed

….parseArguments to unit test

cmsbuild removed tests-rejected code-checks-approved labels Aug 8, 2024

cmsbuild modified the milestones: CMSSW_15_0_X, CMSSW_14_2_X Nov 22, 2024

cmsbuild mentioned this pull request Nov 22, 2024

[CORE] py2/3 compatibility:drop use of __future__ #46769

Merged

cmsbuild added tests-started and removed tests-pending labels Nov 27, 2024

makortel mentioned this pull request Nov 27, 2024

Default VarParsing options in runGEMUnpacker_cfg.py do not work #46815

Open

cmsbuild added tests-approved and removed tests-started labels Nov 27, 2024

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Dec 2, 2024

cmsbuild added orp-approved and removed orp-pending labels Dec 3, 2024

cmsbuild merged commit 4ca0f0d into cms-sw:master Dec 3, 2024
11 checks passed

This was referenced Dec 3, 2024

BuildRules: Fix for class version checks cms-sw/cmsdist#9543

Merged

[ROOT6] Updated root to tip of branch master cms-sw/cmsdist#9542

Merged

[DQM] Update compareDQMOutput.py #46854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VarParsing: prevent TypeError for default of int/float/bool list #45640

VarParsing: prevent TypeError for default of int/float/bool list #45640

IzaakWN commented Aug 5, 2024

cmsbuild commented Aug 5, 2024 •

edited

Loading

cmsbuild commented Aug 5, 2024

cmsbuild commented Aug 5, 2024

makortel commented Aug 5, 2024

cmsbuild commented Aug 7, 2024

cmsbuild commented Aug 7, 2024

makortel commented Aug 7, 2024

cmsbuild commented Aug 8, 2024

IzaakWN commented Aug 8, 2024 •

edited

Loading

IzaakWN commented Nov 22, 2024

makortel commented Nov 27, 2024

makortel commented Nov 27, 2024

makortel commented Nov 27, 2024

cmsbuild commented Nov 27, 2024

makortel commented Nov 27, 2024

makortel commented Nov 27, 2024

makortel commented Dec 2, 2024

cmsbuild commented Dec 2, 2024

mandrenguyen commented Dec 3, 2024

VarParsing: prevent TypeError for default of int/float/bool list #45640

VarParsing: prevent TypeError for default of int/float/bool list #45640

Conversation

IzaakWN commented Aug 5, 2024

PR description:

PR validation:

PR tests

cmsbuild commented Aug 5, 2024 • edited Loading

cmsbuild commented Aug 5, 2024

cmsbuild commented Aug 5, 2024

makortel commented Aug 5, 2024

cmsbuild commented Aug 7, 2024

cmsbuild commented Aug 7, 2024

makortel commented Aug 7, 2024

cmsbuild commented Aug 8, 2024

Unit Tests

Comparison Summary

IzaakWN commented Aug 8, 2024 • edited Loading

IzaakWN commented Nov 22, 2024

makortel commented Nov 27, 2024

makortel commented Nov 27, 2024

makortel commented Nov 27, 2024

cmsbuild commented Nov 27, 2024

Comparison Summary

makortel commented Nov 27, 2024

makortel commented Nov 27, 2024

makortel commented Dec 2, 2024

cmsbuild commented Dec 2, 2024

mandrenguyen commented Dec 3, 2024

cmsbuild commented Aug 5, 2024 •

edited

Loading

IzaakWN commented Aug 8, 2024 •

edited

Loading