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 overrides without . #4

Open
adrianeboyd opened this issue Nov 7, 2022 · 9 comments
Open

Allow overrides without . #4

adrianeboyd opened this issue Nov 7, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@adrianeboyd
Copy link
Contributor

Mainly as a note for the future because it came up in the check_requirements overrides for spacy project:

Is there a technical limitation that requires confection to only support overrides that contain . or could this be removed to also support top-level overrides?

@rmitsch rmitsch added the enhancement New feature or request label Nov 7, 2022
@rmitsch rmitsch self-assigned this Nov 7, 2022
@rmitsch
Copy link
Contributor

rmitsch commented Nov 8, 2022

Context: explosion/spaCy#11735.

I had a look and bypassing this wouldn't be trivial. Config values without sections (such as check_requirements) are not supported by confection, as they are parsed as improperly formatted. This comment provides motivation.

So while possible, it would be a redesign of how sections are processing by confection. I agree that it's not satisfactory not being able to override top-level properties. I'm inclined to say that this is probably not worth it though (maybe I'm also missing an easy workaround).

@adrianeboyd
Copy link
Contributor Author

adrianeboyd commented Nov 9, 2022

Maybe then it makes sense to redesign how the Config is used in weasel so that everything is put into a single top-level section and overrides are automatically adjusted?

@rmitsch
Copy link
Contributor

rmitsch commented Nov 9, 2022

Yes, I think that makes sense. Pinging @polm to get his opinion.

@polm
Copy link

polm commented Nov 9, 2022

I think it makes sense to have this as a feature if it's feasible to implement it. Putting everything in a single top-level section sounds like it makes sense, but won't that require rewriting all the sections in the config file?

It seems like a common workaround is to insert a filler section at the top, like [root], so that things outside a section end up in that. But then we would get weird errors if a user added a section with the same name.

@adrianeboyd
Copy link
Contributor Author

I think this would just be for internal processing of the project config, so there isn't any danger of collisions? As in here:

https://github.com/explosion/spaCy/blob/03eebe9d1c79d39a632876205e93f023fc096d85/spacy/cli/_util.py#L135

@rmitsch
Copy link
Contributor

rmitsch commented Nov 9, 2022

...but won't that require rewriting all the sections in the config file?

We can either add a level of hierarchy altogether or gather all top-level attributes in one made-up section and treat that as a special case? I'm leaning towards the former.

@adrianeboyd
Copy link
Contributor Author

Oh, it is already put into "project", hmm. Then I'd have to look again at how the overrides are done.

@polm
Copy link

polm commented Nov 9, 2022

OK, I see what's going on. What it currently does is read in the config, then create a new object where the config is under the key "project", and then goes from there. So the rewriting is part of the normal serialization process, and because default values aren't written out it doesn't change anything. That happens here:

https://github.com/explosion/spaCy/blob/03eebe9d1c79d39a632876205e93f023fc096d85/spacy/cli/_util.py#L195

@rmitsch
Copy link
Contributor

rmitsch commented Nov 10, 2022

Shall we close this and open an issue for weasel instead?

@adrianeboyd adrianeboyd transferred this issue from explosion/confection Nov 10, 2022
@rmitsch rmitsch removed their assignment Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants