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 passing list type arguments to k4run easier and add tests to verify parsing #164

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tmadlener
Copy link
Contributor

BEGINRELEASENOTES

  • Make it possible to pass lists as comma separated strings as arguments to k4run for Gaudi::Property<vector<T>> algorithm properties. This is a breaking change, previously the same was possible with adding to the list by passing the argument multiple times with different values
    • Enable this also for properties for which this was previously disabled because the type could not be determined from the default value. In this case make it a list of strings.
  • Add an algorithm for testing that this parsing works as expected and some test cases using this algorithm.

ENDRELEASENOTES

Although this is a breaking change, I think this is an improvement to the current situation because

  • it makes it easier to pass list arguments via the command line which is currently quite cumbersome
  • it makes it possible to pass in a list of strings to a vector<string> property that is defaulted to be empty. What makes this special is that e.g. the PodioInput.collections properties is such a list of strings and currently it would be impossible to change that from the command line.

I am not sure how many actual workflows this currently breaks. I think / hope not too many. In the end my major motivation at the moment is actually for testing, where this would be extremely handy to avoid having to write a second options file just to change effectively one list of values.

@tmadlener
Copy link
Contributor Author

As just discussed during the Key4hep meeting, we might want to go in the complete opposite direction with k4run, i.e. simplifying the argument parsing and removing the possibility to touch all the parameters of all the algorithms, in favor of using k4FWCore.parseArgs.parser to implement the necessary parsing only for the specific parameters that are necessary, e.g.

from k4FWCore.parseArgs import parser

parser.add_arguments("--algoParam", help="My special parameter")
my_args = parser.parse_known_args()[0]

# ...
myAlgo.specialParameter = my_args.algoParam

I will mark this as draft in the meantime and open an issue to not forget about documenting the possibility of using argparse to handle command line arguments in option files.

@tmadlener tmadlener marked this pull request as draft November 29, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant