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

Unable to fill config if using *args #47

Closed
MartinBernstorff opened this issue Nov 15, 2023 · 6 comments · Fixed by #53
Closed

Unable to fill config if using *args #47

MartinBernstorff opened this issue Nov 15, 2023 · 6 comments · Fixed by #53
Labels
bug Something isn't working

Comments

@MartinBernstorff
Copy link

MartinBernstorff commented Nov 15, 2023

I'd like to do the filling before resolution, because then I can overwrite the .cfg with the filled values. A minimal reproducible example below.

@MartinBernstorff
Copy link
Author

MartinBernstorff commented Nov 15, 2023

A minimal reproducible example:

from dataclasses import dataclass

import catalogue
from confection import Config, registry


class Registry(registry):
    registry = catalogue.create("psycop", "test_reg")

@dataclass(frozen=True)
class DataClass:
    pass

@Registry.registry.register("test_fn")
def test_fn(*args: DataClass):
    pass

@Registry.registry.register("dataclass_factory")
def dataclass_factory() -> DataClass:
    return DataClass()

config_str = """
[test_fn]
@registry = "test_fn"

[test_fn.*.test_arg]
@registry = "dataclass_factory"
"""

if __name__ == "__main__":
    cfg = Config().from_str(text=config_str)
    filled = Registry.fill(cfg)

Further experiments

It seems that running .fill also removes any *-arguments.

if __name__ == "__main__":
    cfg = Config().from_str(text=config_str)
    filled = Registry.fill(cfg, validate=False)
> cfg
{'test_fn': {'@registry': 'test_fn', '*': {...}}}

> filled
{'test_fn': {'@registry': 'test_fn'}}

@KennethEnevoldsen
Copy link
Contributor

So a bit of experimenting. First of all simplified the example a bit (still breaks):

import catalogue
from confection import Config, registry


class Registry(registry):
    registry = catalogue.create("psycop", "test_reg")


@Registry.registry.register("test_fn")
def test_fn(*args: int):
    return 2


config_str = """
[test_fn]
@registry = "test_fn"

[test_fn.*.test_arg]
arg=1
"""

if __name__ == "__main__":
    cfg = Config().from_str(text=config_str)
    Registry.fill(cfg, validate=True) # does not work

Seems like avoiding validation works just fine:

cfg = Config().from_str(text=config_str)
Registry.resolve(cfg) # works (sanity check)
cfg_filled = Registry.fill(cfg, validate=False) # works

but resolving the filled cfg does not work:

Registry.resolve(cfg_filled) # does not work
Registry.fill(cfg, validate=True) # does not work (what you want)

this seems to be due to the cfg having the following invalid format:

cfg_filled = {'test_fn': {'@registry': 'test_fn'}}
# as opposed to:
cfg = {'test_fn': {'@registry': 'test_fn', '*': {'test_arg': {'arg': 1}}}}

Where it seemingly deleted an argument during filling.

@KennethEnevoldsen
Copy link
Contributor

Errors seem to arise from the line:

                result = schema.parse_obj(validation)

Which seeks to parse:

{'@registry': 'test_fn', '*': [{'arg': 1}]}

Where the schema is defined as:

print(schema.__fields__)
{'@registry': ModelField(name='@registry', type=str, required=True), 'a': ModelField(name='a', type=Optional[Any], required=False, default=1), 'VARIABLE_POSITIONAL_ARGS': ModelField(name='VARIABLE_POSITIONAL_ARGS', type=Sequence[int], required=True, alias='*')}

It seems to expect that [{'arg': 1}] is a Sequence[int]

@KennethEnevoldsen
Copy link
Contributor

KennethEnevoldsen commented Nov 15, 2023

As for the filtering of "*" when validate=False i happen here:

Due to "*" not being in the allowed fields but rather VARIABLE_POSITIONAL_ARGS, my guess is that "*" needs to be mapped to VARIABLE_POSITIONAL_ARGS to avoid filtering it as an extra argument.

@svlandeg
Copy link
Member

Thanks for the report(s)! We'll look into this.

@KennethEnevoldsen
Copy link
Contributor

@svlandeg added a PR for solving this issue.

@ines ines closed this as completed in #53 Dec 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants