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

VarParsing default doesn't work for lists? #45961

Open
gpetruc opened this issue Sep 9, 2024 · 7 comments
Open

VarParsing default doesn't work for lists? #45961

gpetruc opened this issue Sep 9, 2024 · 7 comments

Comments

@gpetruc
Copy link
Contributor

gpetruc commented Sep 9, 2024

Hi,

The behaviour of VarParsing for lists doesn't seem right to me,

    def register (self, name,
                  default = "",
                  mult    = multiplicity.singleton,
                  mytype  = varType.int,
                  info    = "",
                  **kwargs):
            [....]
            # if it's a list, we only want to use the default if it
            # does exist.
            if len (default):
                self._lists[name].append (default)

The first line checks whether the provided default is a non-empty list, but then the second appends the whole list as entry in the default. As a result, what one gets as default value is not what is passed as default to register, but rather a list of lists, with one single entry that is equal of lists.

What would make more sense to me if the line were to be self._lists[name] += default.

As is I can't call register with as default a non-empty list (except in the case where it's a list of strings containing just one entry, as in that case I can pass the entry value as default, and len() will act on the string)

I can make a PR, but git blame records the code as being 11 years old, so maybe this is the intended behaviour and I'm not understanding the logic for it?

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2024

A new Issue was created by @gpetruc.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

assign core

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2024

New categories assigned: core

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

@gpetruc
Copy link
Contributor Author

gpetruc commented Sep 9, 2024

Code https://github.com/cms-sw/cmssw/blob/master/FWCore/ParameterSet/python/VarParsing.py#L416

Reproducer code

import FWCore.ParameterSet.Config as cms
import FWCore.ParameterSet.VarParsing as VarParsing

options = VarParsing.VarParsing ('analysis')
options.register ('buNumStreams',
                  [1], # default value
                  VarParsing.VarParsing.multiplicity.list,
                  VarParsing.VarParsing.varType.int,          # string, int, or float
                  "Number of input streams (i.e. files) used simultaneously for each BU directory")

options.parseArguments()
print(f"numStreams set to '{options.buNumStreams}', len {len(options.buNumStreams)}, type of first element is {type(options.buNumStreams[0])}")

process = cms.Process("NULL")

if run with
python3 demo.py buNumStreams=4,5
it works fine printing
numStreams set to '[4, 5]', len 2, type of first element is <class 'int'>,
but if I don't give a command line argument it prints out the incorrect
numStreams set to '[[1]]', len 1, type of first element is <class 'list'>
while the expected behaviour would be
numStreams set to '[1]', len 1, type of first element is <class 'int'>

@Dr15Jones
Copy link
Contributor

Although this code is in FWCore.ParameterSet, it really isn't maintained anymore. Instead, we suggest people migrate the the python standard argparse as work was done to make that easier to use with cmsRun.

That being said, we do accept changes done to VarParsing committed by others.

@makortel
Copy link
Contributor

makortel commented Sep 9, 2024

This looks like the same issue reported already in #44630, and for which there is a PR open #45640 to try to improve the situation (see especially #45640 (comment)).

I can make a PR, but git blame records the code as being 11 years old, so maybe this is the intended behaviour and I'm not understanding the logic for it?

I believe the knowledge on "intended behavior" of VarParsing is long gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants