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

Allow config overrides to add new keys #527

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

ines
Copy link
Member

@ines ines commented Aug 31, 2021

Still need to test this against spaCy to check if we need to adjust any tests here. Also a bit worried about potential unitended side-effects (or some important reason why we included this feature that I forgot).

Related: explosion/spacy-transformers#283 (comment)


The config system currently only allows passing on overrides for existing settings, not new keys. For instance, given a config like this:

[section]
foo = 1
bar = 2

Passing in the overrides {"section.foo": 3} is valid, whereas {"section.baz": 3} would raise an error. In a lot of scenarios, this makes sense, because you'd expect the config to be complete and describing all available settings – so anything unexpected is likely a problem. But there are other cases where the config is more flexible (e.g. a dictionary with arbitrary keys passed to a function as an argument).

@ines ines added enhancement Feature requests and improvements feat / config Configuration system and config files labels Aug 31, 2021
@svlandeg
Copy link
Member

I had a look into the history and apparently you had it like this in your original PR, but then changed it: ac1e9ca

There wasn't another reason discussed other than structuring the code differently.

I wonder whether it would make sense to warn when a new key is defined though. To guard the user against making a typo, accidentally adding a new key and not updating the old one as they would have expected.

@ines ines marked this pull request as ready for review September 3, 2021 03:36
@svlandeg svlandeg merged commit 35ac30e into master Sep 3, 2021
@svlandeg svlandeg deleted the feature/config-overrides-new-values branch September 3, 2021 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements feat / config Configuration system and config files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants