Support parameterising study definitions

[evansd]

This teaches generate_cohort to accept a new, multi-valued --param argument e.g.

cohortextractor generate_cohort --param key1=value1 --param key2=value2 --param key3

The study definition can access these via the params dict in the cohortextractor module:

from cohortextractor import params
print(params)
# {'key1': 'value1', 'key2': 'value2', 'key3': ''}

There was a suggestion earlier of using --arg for this, but I think --param avoids confusion with other kinds of thing which are also called "arguments", and fits neatly with the term "parameterised study definition".

This was all much harder than it ought to be because implicit in this feature is the need for the output file name to be customised. Previously the output directory and format could be changed, but the file name was determined by the study definition name e.g. study_definition_test.py would always produce a file called <output_dir>/input_test.<extension>.

So we now support an --output-file argument which sets the directory, base name and file format in one go e.g. some_dir/my_results.feather.

[rebkwok]

One typo and some questions that are all along the lines of "what happens if a user does this weird thing"; feel free to ignore if they're irrelevant

[rebkwok]

urgh

[rebkwok]

👏

[wjchulme]

This is fab! Couple of questions:

If I do --param arg1 = 1 in the action and arg1 = params["arg1"] in the study def, is arg1 a string "1" or a numeric 1? What about arg1 = True? string or logical? I'm thinking of scenarios where we need the non-string type in the study definition, which might catch users out. For example I don't think find_first_match_in_period = "True" will work (with True as a string not a bool), so we'd need to do arg1 = params["arg1"]=="True" or something like that to get the correct type.

Can I do --param key1 = "string with white space" ?

[sebbacon]

🌊 🐦 : have you done documentation and "platform news" for this? Cos it's cool.

[evansd]

Thanks @wjchulme. This is interesting feedback because it already exposes one misunderstanding which is that you can't write key = value, it has to be key=value because of the vagaries of bash's argument parsing (the former is considered to be three separate arguments, and would need to be quoted as 'key = value' if you wanted it treated as a single one). I think command line folk get so used to this that they forget not everyone has the same form of Stockholm syndrome :)

That said, I think we can make the space separated version work with a bit of fiddling. We'd need to let --param accept an arbitrary number of arguments and then join them together before parsing them. And given that you won't be the only person to use spaces I think we'll have to support that as well.

You can definitely include whitespace. If we make the change above then your example would work exactly as written. Otherwise you'd have to write it as --param 'key1=string with whitespace'.

As for types, there isn't a neat solution with this approach. I think the only sane option is to make it clear that everything is a string and needs to be explicitly converted if you want something else.

There is a totally different approach we could take though that would give type support, and that's to use the config: option in project.yaml. That would let you write something like:

generate_cohort:
  run: cohortextractor:latest generate_cohort --study-definition study_definition
  config:
    param_1: some string
    param_2: true
    param_3: 10
  outputs:
    highly_sensitive:
      cohort: output/input.csv

I actually think that would be less work than trying to add the support for spaces I described above. So maybe that's the way to go?

[evansd]

have you done documentation and "platform news" for this? Cos it's cool.

No, but was planning to do the docs once it was merged. And maybe the platform news after at least one person has tried it first?

[wjchulme]

everything is a string and needs to be explicitly converted if you want something else

This is how the equivalent functionality works in R, so at least there's some consistency. The problem is that most users won't know how to do the conversion in python. These are edges cases though -- I reckon a vast majority of parameters used will be needed as strings (even if conceptually they're numeric, eg f"date + {param1} days").

[wjchulme]

I think users would quickly get used to having to use key=value, not key = value. We could just advise that all key-value pairs should be expressed inside quotes to avoid problems.

I do like the config: suggestion though. It's probably less error-prone. More sturdy.

[evansd]

I do like the config: suggestion though. It's probably less error-prone. More sturdy.

Yeah, I go a bit back and forth on this. It relies on coupling bits of the system together in ways which make me a bit uncomfortable. Not saying we definitely shouldn't do it, but I'd rather avoid baking that behaviour in at this stage.

For now I've just kept the --param syntax but implemented the whitespace handling.