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

Enable ability to override list values in config via CLI #1430

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wizeng23
Copy link
Contributor

@wizeng23 wizeng23 commented Feb 13, 2025

Description

Problem

Previously, we could not override config values that were within a list field via the CLI, ex. oumi evaluate -c configs/recipes/smollm/evaluation/135m/eval.yaml --tasks[0].num_samples=1, getting the error omegaconf.errors.ConfigTypeError: Cannot merge DictConfig with ListConfig. This is not a problem with using the wrong CLI syntax, but instead with how we were parsing the CLI overrides.

Currently, we create a config from the args list with OmegaConf.from_cli(arg_list), and then merge it with the YAML config via OmegaConf.merge(*all_configs). The YAML config is an OmegaConf structured config, meaning it's aware of the typing of our Config object, EvaluationConfig in this example. The problem is that the config from args list is created before it's merged with the YAML config, so it is not aware of the typing. In the above example with --tasks[0].num_samples=1, it's thus not aware that tasks is a List, and creates a dict instead: {"tasks": {"0": {"num_samples": 1}}}

Solution

The solution is simply to call config.merge_with_dotlist(arg_list) instead, which merges each argument into the config individually. With this method, the 0 is properly interpreted as a list index instead of a dict key.

I've tested that this does not impact existing behavior. I've also verified that many variants of specifying overrides, which are Pythonic and we expect to work, do work. Here's some example args for the config mentioned in the first example:

# Bracket notation
--tasks[0].num_samples=1
# Dot notation
--tasks.0.num_samples=null
# Override existing key in dict
--tasks.0.eval_kwargs.num_fewshot=1
# Add new key in dict
--tasks.0.eval_kwargs.foo=bar
# Merge dict values within the list entirely
# Note this doesn't override the dict, as OmegaConf merges dicts by default
--tasks.0.eval_kwargs="{'foo':'bar', 'baz':2}"

NOTE: This PR doesn't support the ability to add new elements in the list. This is currently not supported by OmegaConf: facebookresearch/hydra#1547 (comment)

Related issues

Fixes OPE-934

Before submitting

  • This PR only changes documentation. (You can ignore the following checks in that case)
  • Did you read the contributor guideline Pull Request guidelines?
  • Did you link the issue(s) related to this PR in the section above?
  • Did you add / update tests where needed?

Copy link
Collaborator

@taenin taenin left a comment

Choose a reason for hiding this comment

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

Can you add tests for the cases that were previously failing?

Copy link
Contributor

@oelachqar oelachqar left a comment

Choose a reason for hiding this comment

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

Would be great to add a doc page or section explaining this!

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.

3 participants