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

Make cmsRun-compatible argparse parser #34469

Closed
kpedro88 opened this issue Jul 13, 2021 · 10 comments
Closed

Make cmsRun-compatible argparse parser #34469

kpedro88 opened this issue Jul 13, 2021 · 10 comments

Comments

@kpedro88
Copy link
Contributor

Currently, the "official" recommendation to enable command-line arguments in cmsRun configs is VarParsing. This is a very old and limited parser, with strange behavior (e.g. assigning to a VarParsing.multiplicity.list object actually appends to it). However, it does handle the difference in sys.argv content when a config is executed with python vs. with cmsRun.

Having a drop-in extension of argparse's parser would be a useful improvement to this situation. argparse offers significantly more functionality and is more familiar to incoming developers who have experience with python outside of CMSSW.

See also: #29338

attn: @davidlange6

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 13, 2021

A new Issue was created by @kpedro88 Kevin Pedro.

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

cms-bot commands are listed here

@makortel
Copy link
Contributor

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

@kpedro88
Copy link
Contributor Author

Apparently there is already some support for this: https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideAboutPythonConfigFile#argparse

Encapsulating this behavior in a derived class like cmsArgumentParser (with tests) and providing more usage examples would be helpful for users.

@kpedro88
Copy link
Contributor Author

Following up from Core Software meeting:

With that in mind, as @Dr15Jones requested, I've created a draft PR that modifies cmsRun directly to support argparse: #42650

Some items to consider:

  • The two interface changes are detailed in the PR description.
  • These interface changes imply that tests in 15+ other packages need to be updated. Mostly easy to do, but not done yet. Maybe better to have a separate branch/PR with all of those test updates, and then test them together? (for review purposes)
  • The combination of the above points suggests difficulty for backporting this change to other release cycles, as nice as the consistency would be...

Feedback welcome!

@kpedro88
Copy link
Contributor Author

kpedro88 commented Jan 3, 2024

This is done now (#42650, #42798, #42823, #43042, #43102, #43116). @cms-sw/core-l2 can you sign and close?

(edit: and also dmwm/CRABServer#7888, dmwm/CRABClient#5210 (ref. dmwm/CRABClient#5208))

@makortel
Copy link
Contributor

makortel commented Jan 3, 2024

+core

@makortel
Copy link
Contributor

makortel commented Jan 3, 2024

@cmsbuild, please close

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 3, 2024

This issue is fully signed and ready to be closed.

@kpedro88
Copy link
Contributor Author

For future reference when considering similar interface changes: the site tests from computing should also be considered as a separate user/stakeholder. (They did use -p, even though production did not; they have now been updated for the new interface.)

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

3 participants