fixes #275

[o-smirnov]

@landmanbester could you please test that this branch does not break pfb-clean CLIs?

The change is that policies.repeat is now properly honoured for List and Tuple types, to accommodate the different styles of passing multiple values. As per documentation:

• repeat: list uses --option X Y
• repeat: repeat uses --option X --option Y
• repeat: '[]' uses --option [X,Y]
• repeat: ',' (or any other separator string) uses --option X,Y

There is no default for the repeat policy, and clickify_parameters will raise an error if this is not explicitly specified for List and Tuple types. This may perhaps be an annoyance, and I'm open to using a default instead, but which one?

[JSKenyon]

This looks fine to me bar the debugging relic but I will leave it to Landman to test in earnest.

[landmanbester]

Sorry, got distracted by FFTs. Something is not working. If I have the following in a config file

inputs:
  ms:
    dtype: List[URI]
    required: true
    abbreviation: ms
    info:
      Path to measurement set
 
outputs:
    {}

I now get

(pfbtest) ╭─landman@LAPTOP-ED4N50Q2 ~/software
╰─➤  pfb --help
Traceback (most recent call last):
  File "/home/landman/venvs/pfbtest/bin/pfb", line 33, in <module>
    sys.exit(load_entry_point('pfb-clean', 'console_scripts', 'pfb')())
  File "/home/landman/venvs/pfbtest/bin/pfb", line 25, in importlib_load_entry_point
    return next(matches).load()
  File "/usr/lib/python3.10/importlib/metadata/__init__.py", line 171, in load
    module = import_module(match.group('module'))
  File "/usr/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/home/landman/software/pfb-clean/pfb/workers/main.py", line 10, in <module>
    from pfb.workers import (init, grid, degrid,
  File "/home/landman/software/pfb-clean/pfb/workers/init.py", line 21, in <module>
    @clickify_parameters(schema.init)
  File "/home/landman/venvs/pfbtest/lib/python3.10/site-packages/scabha/schema_utils.py", line 218, in clickify_parameters
    raise SchemaError(f"list-type parameter '{name}' does not have a repeat policy set")
scabha.exceptions.SchemaError: list-type parameter 'ms' does not have a repeat policy set

Should I change something in the config?

[landmanbester]

Adding the following

inputs:
  ms:
    dtype: List[URI]
    required: true
    abbreviation: ms
    info:
      Path to measurement set
    policies:
      repeat: '[]'

fixes the problem but it is a little annoying having to add it for every parameter. I kind of like repeat: list as the default, seems the most natural

[o-smirnov]

I kind of like repeat: list as the default, seems the most natural

Not really. That would be --option X1 X2 X3 ... on the command line -- click doesn't even support that for dashed options, because it becomes poorly defined w.r.t. whether X2 or X3 refers to the option, or to an extra positional argument.

I would prefer repeat: repeat (--option X1 --option X2) or repeat: ',' (--option X1,X2) as the default. Pick one?

Note that it's a a completely separate question from what should be the default when going in the opposite direction (converting a stimela params dict to CLI arguments). Currently, there is a hard check for an explicit repeat policy here. I would argue that this should stay in place.

Which raises a related question. Cabs have an optional cab-level policies section, which sets the default policies for all the parameters of the cab. I suspect this the reason the pfb-clean cabs have worked without an explicit repeat policy for each List-type parameter. Going in the opposite direction, via clickify_parameters(), there is no way to specify default policies. Should we add the possibility of specifying such defaults?

[landmanbester]

click doesn't even support that for dashed options, because it becomes poorly defined w.r.t. whether X2 or X3 refers to the option, or to an extra positional argument.

Guess I'm blind to this because of my aversion to positional arguments. It is very handy that clickify_parameters makes it possible to pass in arbitrary length lists.

I would prefer repeat: repeat (--option X1 --option X2) or repeat: ',' (--option X1,X2) as the default. Pick one?

I would prefer the latter but it's not a very strong preference.

Should we add the possibility of specifying such defaults?

This would be handy. Maybe as a keyword argument to clickify_parameters? Alternatively, I guess I could also add it to this file if it's easier?

[o-smirnov]

All right, I also vote repeat: ',' (--option X1,X2) as the default. I shall make it thus.

This would be handy. Maybe as a keyword argument to clickify_parameters?

I shall make it thus.

[landmanbester]

Awesome, thanks. Once you have it going I will test the changes in earnest and finish my review

[o-smirnov]

All right, please test and re-review so we can merge this.

[landmanbester]

Did you mean to leave this in here?

[landmanbester]

All seems to be working in pfb, thanks. Just that one stray comment that you might not want to leave in there

[o-smirnov]

Thanks @landmanbester. Please reapprove, my last commit invalidated your review.