riga / law

Build large-scale task workflows: luigi + job submission + remote targets + environment sandboxing using Docker/Singularity
http://law.readthedocs.io
BSD 3-Clause "New" or "Revised" License
98 stars 41 forks source link

[Feature Request] `ConfigParameter` #124

Closed pfackeldey closed 1 year ago

pfackeldey commented 2 years ago

Dear @riga,

as discussed offline a ConfigParameter, similar to this one would be a great addition to law, especially for e.g. Deep Learning applications.

Best, Peter

riga commented 2 years ago

Hey @pfackeldey ,

turns out that --some-parameter.foo bar --some-parameter.baz testself.some_parameter = {"foo": "bar", "baz": "test"} is not that simple to implement in law with an intuitive interface. Three things come to mind:

  1. For this to be really beneficial, one would need proper type casting so that e.g. --some-parameter.a-flag would directly translate to a bool, --some-parameter.some-int 1 to an integer, etc. This requires to define the expected sub fields before hand, and for that one could simply introduce a set of namespace-like parameters on the user side, e.g. --nn-layers INT, --nn-activation STR, and then gather them manually in a dictionary in the task init. I think that without this type conversion, this feature would feel quite incomplete.

  2. Without this a-priori definition of subfields and type conversion, one would rather implement this in luigi itself, in particular at the interface with argparse. In there, after argparse handled all known parameters, one could manually treat all unknown parameters, in that one should check if some of them have a common prefix, some-parameter above, and then convert them into a dict (without type conversion). I'm not sure whether this could be done in a clean way.

  3. Internally in luigi, the cli parameter format --A.B-C D is reserved for setting parameter C of task family A.B to D. In the example above, I suppose --some-parameter.some-int would be interpreted as task namespace some_parameter, task class some and parameter int. This is another thing to check, but I suspect this is nothing one wants to change (and also the auto-completion in law would not be happy about that ;) ).

Did you see https://luigi.readthedocs.io/en/stable/api/luigi.parameter.html#luigi.parameter.DictParameter?

riga commented 1 year ago

Closing this for now. Feel free to reopen in case you want to discuss more :)