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

Improve configuration parameter type compatibility checks in python #41356

Closed
makortel opened this issue Apr 17, 2023 · 6 comments
Closed

Improve configuration parameter type compatibility checks in python #41356

makortel opened this issue Apr 17, 2023 · 6 comments

Comments

@makortel
Copy link
Contributor

This issue is a follow-up to discussion in #41349 (comment)

Currently assignment like

process.options.numberOfThreads = cms.untracked.int32(4)

override the type of the existing parameter of a PSet. In most cases the incorrect type gets caught by the C++ code (in case of #32070 it wasn't caught, which is fixed in #41349). It would be generally beneficial to be able catch such inconsistencies already earlier in python.

For regular cms.X types @Dr15Jones came with three options

  1. do an implicit type conversion. I.e. if the value in the ‘wrong’ type is convertible to the ‘right’ type, do the conversion quietly
  2. check for compatibility when assigning to ‘special’ types, i.e. if assigning to optional, required or allowed enforce that the type is proper
  3. require ALL types match. If one wants to ‘change’ a type they must first do a del pset.name to get rid of it
    • or we could add new override proxy type, making intentional type changes to look like p.a = cms.override.int32(4)

Since the PSet assignment is really handled by PSet.__setattr__(), the necessary changes would need to go there, more specifically to

# handle the case where users just replace with a value, a = 12, rather than a = cms.int32(12)
if isinstance(value,_ParameterTypeBase):
self.__dict__[name] = value
else:
self.__dict__[name].setValue(value)

@makortel
Copy link
Contributor Author

assign core

@cmsbuild
Copy link
Contributor

New categories assigned: core

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

@cmsbuild
Copy link
Contributor

A new Issue was created by @makortel Matti Kortelainen.

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

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

The improvements were done in #41361

@makortel
Copy link
Contributor Author

+core

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

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

2 participants