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

Make runners configurable from the command-line using hydra #49

Closed
wants to merge 37 commits into from

Conversation

charlesbmi
Copy link
Contributor

@charlesbmi charlesbmi commented Oct 11, 2023

#44

Introduction

This PR adds command-line configuration to the run_*.py scripts. For example you can now change the logger level with:
streamer log_level=DEBUG.

Changes

  • Switch from pydantic-yaml library to hydra. hydra both reads in YAML config files and parses any values from the command-line interface
    • The config files (mostly) did not need to change for this update.
  • Replaces deprecated YamlStrEnum by str, Enum mix-inheritance

This hydra/PyDantic integration is based on this blog post: https://suneeta-mall.github.io/2022/03/15/hydra-pydantic-config-management-for-training-application.html#can-pydantic-bridge-the-gaps-in-hydra

Behavior

  • run_closed_loop works the same:
    Screen Recording 2023-10-11 at 4 23 19 PM 2
  • The run_*.py scripts can now be configured on the command-line, e.g. streamer log_level=DEBUG streamer.stream_indefinitely=false
  • run_*.py --help shows the default config, e.g.:
run_streamer is powered by Hydra.

== Configuration groups ==
Compose your configuration from those groups (group=option)



== Config ==
Override anything in the config (foo.bar=value)

version: 1.0.0
log_level: INFO
streamer:
  lsl_chunk_frequency: 100
  input_type: npz
  stream_indefinitely: true
  blackrock:
    input:
      file: sample_data/blackrock-file.ns6
    output:
      lsl:
        channel_format: int16
        instrument:
          manufacturer: Blackrock Neurotech
          model: Playback
          id: 1
  npz:
    input:
      file: sample_data/session_4_behavior_standardized.npz
      timestamps_array_name: timestamps_train
      data_array_name: vel_train
    output:
      sampling_rate: 50
      n_channels: 2
      lsl:
        channel_format: float32
        stream_name: NDS-Behavior
        stream_type: Behavior
        source_id: SimulatedBehavior
        instrument:
          manufacturer: Blackrock Neurotech
          model: Simulated
          id: 0
  • The scripts do also generate an outputs folder, which is mostly unused right now.

@charlesbmi charlesbmi added the enhancement New feature or request label Oct 11, 2023
@charlesbmi charlesbmi self-assigned this Oct 11, 2023
@charlesbmi charlesbmi linked an issue Oct 11, 2023 that may be closed by this pull request
@charlesbmi charlesbmi marked this pull request as ready for review October 20, 2023 00:01
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Files Coverage Δ
src/neural_data_simulator/core/settings.py 94.51% <100.00%> (+0.20%) ⬆️
src/neural_data_simulator/streamer/settings.py 95.83% <100.00%> (ø)
src/neural_data_simulator/scripts/run_encoder.py 83.47% <75.00%> (-2.89%) ⬇️
...ural_data_simulator/scripts/run_ephys_generator.py 84.15% <75.00%> (-2.94%) ⬇️
src/neural_data_simulator/streamer/run_streamer.py 92.85% <80.00%> (-2.57%) ⬇️
src/neural_data_simulator/tasks/run_closed_loop.py 24.71% <20.00%> (+0.30%) ⬆️
src/neural_data_simulator/decoder/run_decoder.py 47.61% <52.63%> (-1.51%) ⬇️
...eural_data_simulator/tasks/run_center_out_reach.py 36.29% <57.14%> (+2.67%) ⬆️

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@charlesbmi
Copy link
Contributor Author

@NewtonSander have you seen this error before? https://github.com/agencyenterprise/neural-data-simulator/actions/runs/6583346051/job/17886339278?pr=49

On (test) windows-latest:

Run poetry install --no-interaction --no-root

Command ['D:\\a\\neural-data-simulator\\neural-data-simulator\\.venv\\Scripts\\python.exe', '-I', '-W', 'ignore', '-'] errored with the following return code 103

Error output:
No Python at '"C:\hostedtoolcache\windows\Python\3.[11](https://github.com/agencyenterprise/neural-data-simulator/actions/runs/6583346051/job/17886339278?pr=49#step:8:12).5\x64\python.exe'

My PR doesn't touch any actions, so I'm a little confused where this is coming from, unless windows-latest was updated and it broke our tests somehow?

@NewtonSander
Copy link
Collaborator

@NewtonSander have you seen this error before? https://github.com/agencyenterprise/neural-data-simulator/actions/runs/6583346051/job/17886339278?pr=49

On (test) windows-latest:

Run poetry install --no-interaction --no-root

Command ['D:\\a\\neural-data-simulator\\neural-data-simulator\\.venv\\Scripts\\python.exe', '-I', '-W', 'ignore', '-'] errored with the following return code 103

Error output:
No Python at '"C:\hostedtoolcache\windows\Python\3.[11](https://github.com/agencyenterprise/neural-data-simulator/actions/runs/6583346051/job/17886339278?pr=49#step:8:12).5\x64\python.exe'

My PR doesn't touch any actions, so I'm a little confused where this is coming from, unless windows-latest was updated and it broke our tests somehow?

@charlesincharge I wasn't aware of this error, I re-triggered a successful job on main and it also failed.
I've tried to fix the bug in a fork of main and didn't manage to find the root cause, so the workaround was to disable the cache for Windows:
#53

Copy link
Collaborator

@NewtonSander NewtonSander left a comment

Choose a reason for hiding this comment

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

Great job Charles!
The only thing I am missing is the output we had for each command line script when we were passing the --help flag.

@@ -12,7 +12,7 @@ The {class}`decoder.decoders.Decoder` class can also be used with a custom decod

## Configuring and running the included decoder

To configure the decoder, change the file `settings_decoder.yaml`, which is located by default in the `$HOME/.nds/` folder. You can point the script to use a different configuration file by passing the `--settings-path` argument.
To configure the decoder, change the file `settings_decoder.yaml`, which is located by default in the `$HOME/.nds/` folder. You can point the script to use a different configuration file by passing the `--config` argument.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this parameter --config correct? Shouldn't it be --config-path?

Copy link
Contributor Author

@charlesbmi charlesbmi Oct 20, 2023

Choose a reason for hiding this comment

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

Thanks for the catch! Updated to --config-path and --config-name

To use a specific config file, specify the config directory (`--config-dir`) and config file-name (`--config-name`) flags. For example:

```
ephys_generator --config-dir $HOME/.nds/ --config-name settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

have you tried using a --config-dir different from the default NDS_HOME path (os.path.join(os.path.expanduser("~"), ".nds"))?
I think it won't work as the runtime.py has the NDS_HOME variable that is used in the hydra decorators but it's not set by the user input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that.

Looks like --config-path is the correct way to overwrite the config, whereas --config-dir is only appends to the config-search-path. So something like:

streamer --config-path=/Users/charles/Development/nds/src/streamer/config/

still works after removing my ~/.nds/*.yaml configs.

Unfortunately, it requires an absolute path (or a path-relative to streamer, which is probably a bit confusing for most users).

Let me see if there is a way to also pass in a config path relative to the current working directory.

@charlesbmi
Copy link
Contributor Author

Thanks for helping me workaround that test error Newton!

@NewtonSander
Copy link
Collaborator

@charlesincharge it seems like I can't run decoder --help if the config files do not exist in the default location:

decoder --help                    
Traceback (most recent call last):
  File "/Users/nsander/workspace/neural-data-simulator/src/decoder/run_decoder.py", line 110, in run
    run_with_config()
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/main.py", line 94, in decorated_main
    _run_hydra(
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/_internal/utils.py", line 363, in _run_hydra
    hydra.app_help(config_name=config_name, args_parser=args_parser, args=args)
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/_internal/hydra.py", line 356, in app_help
    cfg = self.compose_config(
          ^^^^^^^^^^^^^^^^^^^^
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/_internal/hydra.py", line 594, in compose_config
    cfg = self.config_loader.load_configuration(
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/_internal/config_loader_impl.py", line 142, in load_configuration
    return self._load_configuration_impl(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/_internal/config_loader_impl.py", line 243, in _load_configuration_impl
    self.ensure_main_config_source_available()
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/_internal/config_loader_impl.py", line 129, in ensure_main_config_source_available
    self._missing_config_error(
  File "/Users/nsander/.pyenv/versions/3.11.6/lib/python3.11/site-packages/hydra/_internal/config_loader_impl.py", line 102, in _missing_config_error
    raise MissingConfigException(
hydra.errors.MissingConfigException: Primary config directory not found.
Check that the config directory '/Users/nsander/.nds' exists and readable

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/nsander/.pyenv/versions/3.11.6/bin/decoder", line 6, in <module>
    sys.exit(run())
             ^^^^^
  File "/Users/nsander/workspace/neural-data-simulator/src/decoder/run_decoder.py", line 112, in run
    raise FileNotFoundError(
FileNotFoundError: Run 'nds_post_install_config' to copy the default settings files

after I execute nds_post_install_config I get the yaml config and at the end this message:

Powered by Hydra (https://hydra.cc)
Use --hydra-help to view Hydra specific help

so i have to execute --hydra-help to get the help docs for --config-path and --config-name, but it's mixed with all the other options provided by hydra:

decoder --hydra-help
Hydra (1.3.2)
See https://hydra.cc for more info.

== Flags ==
--help,-h : Application's help
--hydra-help : Hydra's help
--version : Show Hydra's version and exit
--cfg,-c : Show config instead of running [job|hydra|all]
--resolve : Used in conjunction with --cfg, resolve config interpolations before printing.
--package,-p : Config package to show
--run,-r : Run a job
--multirun,-m : Run multiple jobs with the configured launcher and sweeper
--shell-completion,-sc : Install or Uninstall shell completion:
    Bash - Install:
    eval "$(decoder -sc install=bash)"
    Bash - Uninstall:
    eval "$(decoder -sc uninstall=bash)"

    Fish - Install:
    decoder -sc install=fish | source
    Fish - Uninstall:
    decoder -sc uninstall=fish | source

    Zsh - Install:
    Zsh is compatible with the Bash shell completion, see the [documentation](https://hydra.cc/docs/1.2/tutorials/basic/running_your_app/tab_completion#zsh-instructions) for details.
    eval "$(decoder -sc install=bash)"
    Zsh - Uninstall:
    eval "$(decoder -sc uninstall=bash)"

--config-path,-cp : Overrides the config_path specified in hydra.main().
                    The config_path is absolute or relative to the Python file declaring @hydra.main()
--config-name,-cn : Overrides the config_name specified in hydra.main()
--config-dir,-cd : Adds an additional config dir to the config search path
--experimental-rerun : Rerun a job from a previous config pickle
--info,-i : Print Hydra information [all|config|defaults|defaults-tree|plugins|searchpath]
Overrides : Any key=value arguments to override config values (use dots for.nested=overrides)

== Configuration groups ==
Compose your configuration from those groups (For example, append hydra/job_logging=disabled to command line)

hydra: config
hydra/env: default
hydra/help: default
hydra/hydra_help: default
hydra/hydra_logging: colorlog, default, disabled, hydra_debug, none
hydra/job_logging: colorlog, default, disabled, none, stdout
hydra/launcher: basic
hydra/output: default
hydra/sweeper: basic


Use '--cfg hydra' to Show the Hydra config

@charlesbmi
Copy link
Contributor Author

charlesbmi commented Oct 25, 2023

After some discussion with Newton and trying to brainstorm workarounds, I have decided that hydra is probably over-featured and a bit too opinionated for our use-case. For example, hydra is not designed for the situation where the default config file doesn't exist (as it does when the user hasn't run nds_post_install_config)

We do want hydra's specific YAML/command-line-merging logic though, and this is implemented by OmegaConf.
I'll change this PR to use https://omegaconf.readthedocs.io/en/2.3_branch/usage.html#from-command-line-arguments , which should be cleaner and clearer for the user

@charlesbmi
Copy link
Contributor Author

@NewtonSander thanks for the feedback here. I made some substantial changes, so I moved it to a different branch and opened a separate PR: #57

I will close this PR.

@charlesbmi charlesbmi closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request norelease
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make runners configurable from the command-line
3 participants