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

ENH support passing options through config files #325

Merged
merged 24 commits into from May 6, 2022

Conversation

mathurinm
Copy link
Contributor

@mathurinm mathurinm commented Apr 12, 2022

proof of concept to start discussion.

Example of config file: ex.yml

objective-filter:
  - Lasso Regression[fit_intercept=True,reg=0.5]
dataset:
  - simulated
  - leukemia
solver:
  - celer
  - cd
n-repetitions: 1

used with:
benchopt run . --file ex.yml

These config files can be committed on repos for examples, shared across collaborators, are easier to maintain/modify/keep track of when the command to launch the benchmark is long

Questions:

  • do we warn when a CLI option is replaced by an option in the config ?
  • is there a way to avoid hardcoding the replacement of CLI option by specified config options ?
  • discuss names in yml config vs parameters names in benchopt code
  • support forced, pdb, html, do_profile, env_name in the config ?
  • how to use autocompletion for this option?

@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2022

Codecov Report

Merging #325 (8f31b06) into main (9013e53) will decrease coverage by 0.05%.
The diff coverage is 64.00%.

@@            Coverage Diff             @@
##             main     #325      +/-   ##
==========================================
- Coverage   55.58%   55.52%   -0.06%     
==========================================
  Files          42       42              
  Lines        2416     2433      +17     
  Branches      446      451       +5     
==========================================
+ Hits         1343     1351       +8     
- Misses        984      991       +7     
- Partials       89       91       +2     

@jolars
Copy link
Contributor

jolars commented Apr 12, 2022

Great idea! But isn't this syntax more kosher:

objectives:
  lasso:
    fit_intercept: true
    reg: 0.5
datasets:
  - simulated
  - leukemia
solvers:
  - celer
  - cd
repetitions: 1

or even:

objectives:
  - model: lasso
    fit_intercept: true
    reg: 0.5
  - model: slope
    fit_intercept: true
datasets:
  - simulated
  - leukemia
solvers:
  - celer
  - cd
repetitions: 1

do we warn when a CLI option is replaced by an option in the config ?

Shouldnt CLI options take precedence? Or are you talking about defaults?

@mathurinm
Copy link
Contributor Author

The first syntax is a great idea and related to #193, it could be addressed when we progress on the latter.
For the 2nd, since benchmarks are in their own folder, I'm not sure config files should be shared between these folders.

@mathurinm
Copy link
Contributor Author

I pushed a solution for the names differing in config file (where we want to use the name of the click options, e.g. --solver) and what is passed to the click decorated function (a variable solver_names).

To avoid hardcoding the dict, we could rely on click automapping from option to variable name (strip '--', replace '-' by '_') and do the reverse mapping automatically in _get_run_args.

WDYT @tomMoral @TomDLT

benchopt/cli/main.py Outdated Show resolved Hide resolved
@mathurinm
Copy link
Contributor Author

  • Following @jolars suggestion I chose that CLI args override those specified in config_file. click knows when a parameter has its default value, so that's OK.

  • I think in the config file parameters should be passed with their CLI option name (without the --) to ease documentation so it requires a bit of work to match the parameters from config_file with the ones from CLI, because click as already changed their names. Hence I have removed specific variable names in click.option (not an API breaking change), let click do the automatic conversion, and then assign variable names as the output of _get_run_args.

@TomDLT
Copy link
Collaborator

TomDLT commented Apr 13, 2022

I agree with both points.

More generally, is there a good reason to have different names for CLI arguments and the function parameters ?

Support for pdb, env_name, etc. would be nice but is not necessary.
Same for autocompletion.

@mathurinm mathurinm changed the title WIP support passing options through config files ENH support passing options through config files Apr 25, 2022
@mathurinm
Copy link
Contributor Author

I'd like to add an example of config.yml file in the doc, on which page does it belong in your opinion? On the CLI page it doesn't look easy to place it properly (right after the run command?)

@jolars
Copy link
Contributor

jolars commented Apr 25, 2022

I'd like to add an example of config.yml file in the doc, on which page does it belong in your opinion? On the CLI page it doesn't look easy to place it properly (right after the run command?)

Right after the run command sounds fine to me.

@Klopfe
Copy link

Klopfe commented Apr 27, 2022

I agree with Johan, I think after the run command is a good idea

@mathurinm mathurinm requested a review from agramfort May 4, 2022 08:04
Copy link
Member

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very neat! thanks @mathurinm

Could you add at least one test in test_cli.py? I think mimicking the TestRunCmd::test_benchopt_run passing only config should be enough.

Copy link
Member

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

there is just a small leftover --file in the test.

benchopt/tests/test_cli.py Outdated Show resolved Hide resolved
benchopt/tests/test_cli.py Show resolved Hide resolved
@tomMoral tomMoral merged commit e832038 into benchopt:main May 6, 2022
@tomMoral
Copy link
Member

tomMoral commented May 6, 2022

Thanks @mathurinm !!! :)

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.

None yet

6 participants