mintproject / mic

Model Insertion Checker
https://mic-cli.readthedocs.io/en/latest/
5 stars 3 forks source link

Ease how parameters are declared #160

Closed dgarijo closed 4 years ago

dgarijo commented 4 years ago

At the moment we need to declare the parameters in the yaml file and in the config files of the model, plus also set the config files that are being used.

This is too complex. Instead, I propose the following:

This way users don't need to add parameters manually.

issue-label-bot[bot] commented 4 years ago

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.64. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

Cmheidelberg commented 4 years ago

@dgarijo Since I dont know if config files will be standardized to list which values are the user's parameters mic config is looking for any variable enclosed in ${}. There is also no way to know the default value for the parameter based on how the flags are currently set up (since the flag replaces the value)

Example:

chris@chris-ubuntu20:/home/chris/Desktop/tests/simpleModel  (master)$ cat config.json 
{
"input" :
  {
  "dataset_name" : "${config_json}"
},
"output" :
  {
  "path" : "./"
},
"params" :
  {
  "a" : "${a}",
  "b" : "${b}",
  "c" : "${c}"
  }
}
chris@chris-ubuntu20:/home/chris/Desktop/tests/simpleModel  (master)$ cat mic/mic.yaml 
step: 2
name: csa
chris@chris-ubuntu20:/home/chris/Desktop/tests/simpleModel  (master)$ mic encapsulate configs config.json -f mic/mic.yaml 
Added: /home/chris/Desktop/tests/simpleModel/config.json as a configuration file
chris@chris-ubuntu20:/home/chris/Desktop/tests/simpleModel  (master)$ cat mic/mic.yaml 
step: 2
name: csa
configs:
  config_json:
    path: config.json
    format: json
parameters:
  config_json:
    default_value: 0
  a:
    default_value: 0
  b:
    default_value: 0
  c:
    default_value: 0

I am open to any ideas on a better way to implement this. I am also adding a option to stop auto parameter detection with the configs command

dgarijo commented 4 years ago

For default value, it's not possible, I agree, unless we add a syntax like ${a}{default=1} (maybe in next version)

For recognizing inputs as parameters, if you have your inputs already declared in the mic file, then you know they will not be parameters and can be removed. So in your case, the mic file would look like:

chris@chris-ubuntu20:/home/chris/Desktop/tests/simpleModel  (master)$ cat mic/mic.yaml 
step: 2
name: csa
configs:
  config_json:
    path: config.json
    format: json
parameters:
  a:
    default_value: 0
  b:
    default_value: 0
  c:
    default_value: 0

We would need to issue a warning for users telling them to verify the default values, and that's fine

Cmheidelberg commented 4 years ago

I can do that. I will also add documentation to mic encapsulate configs that warns the user to declare their inputs first.

I added warnings:

chris@chris-ubuntu20:/home/chris/Desktop/tests/simpleModel  (master)# mic encapsulate configs config.json -f mic/mic.yaml 
Added: /home/chris/Desktop/tests/simpleModel/config.json as a configuration file
Automatically adding "config_json" as a parameter
Automatically adding "a" as a parameter
Automatically adding "b" as a parameter
Automatically adding "c" as a parameter
Default values will need to be added in mic.yaml for each parameter
dgarijo commented 4 years ago

Perfect! But config_json should not have been added automatically as a parameter because it's already a config!

dgarijo commented 4 years ago

Issue fixed