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

[Bug] Initialisation pydantic BaseModel with List causing validation error #1184

Closed
lalitpagaria opened this issue Dec 5, 2020 · 11 comments
Closed
Labels
bug Something isn't working

Comments

@lalitpagaria
Copy link

lalitpagaria commented Dec 5, 2020

🐛 Bug

Description

First thank you for awesome project.
I am using compose API to initialise class which have only List[str] parameter. When passing config in init pydantic throwing validation error that value is not a valid list (type=type_error.list). I found that config is reading list as ListConfig[str] and pydantic expect it as `List[str].

Please suggest how to instruct compose to force parse config in python's fields.

Checklist

  • [ X ] I checked on the latest version of Hydra
  • [ X ] I created a minimal repro

To reproduce

** Minimal Code/Config snippet to reproduce **
My config.yaml is as follows -

allowed_list:
 - "8080"
 - "8081"

My Model class as follows -

class AllowedPorts(BaseModel):
     allowed_list: List[str]

This is how I am using hydra -

with initialize(config_path="/home/user"):
    cfg = compose("config.yaml")
    allowed_ports = AllowedPort(**cfg)

At allowed_ports = AllowedPort(**cfg), pydantic will throw validation error.
** Stack trace/error message **

// Paste the bad output here!
Traceback (most recent call last):
  File "/Applications/PyCharm CE.app/Contents/plugins/python-ce/helpers/pydev/pydevd.py", line 1477, in _exec
    pydev_imports.execfile(file, globals, locals)  # execute the script
  File "/Applications/PyCharm CE.app/Contents/plugins/python-ce/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18, in execfile
    exec(compile(contents+"\n", file, 'exec'), glob, loc)
...
...

  value is not a valid list (type=type_error.list)
python-BaseException

Expected Behavior

compose should have way to parse config in python default types.

System information

  • Hydra Version : 1.0.4
  • Python version : 3.8
  • Virtual environment type and version : 20.0.31
  • Operating system : MacOS

Additional context

Add any other context about the problem here.

@lalitpagaria lalitpagaria added the bug Something isn't working label Dec 5, 2020
@omry
Copy link
Collaborator

omry commented Dec 5, 2020

Hi, There are some major changes in instantiating in the next version of Hydra (in master now).
Take a look at the doc here.
Specifically, I think using using _convert_ all or partial will solve your problem.

Can you share details about why you need to use pydantic here?

@lalitpagaria
Copy link
Author

Hi, There are some major changes in instantiating in the next version of Hydra (in master now).
Take a look at the doc here.
Specifically, I think using using _convert_ all or partial will solve your problem.

Yes this will solve my problem. Any idea when these will be released?

Can you share details about why you need to use pydantic here?

I am using pydantic because of easy integration with FastApi and less boiler plate code related to validation like http endpoint, ip address, secret string, etc.

Not related to this, is there any plan to support recursive initialisation via instantiate? Let say I have custom class, which have another field as different custom class.

For example

class Class2:
      name:str

class Class1:
       var: Class2

@omry
Copy link
Collaborator

omry commented Dec 5, 2020

Take a look at the docs I linked, the big change is recursive instantiation support. conversion strategies are just just a side thing.

No target data for 1.1. You can follow the milestone progress (there are a lot of things not yet done in the list).

In the mean time you can install from master.
Sometime down there line there will be some dev releases and later release candidate releases.

@lalitpagaria
Copy link
Author

Cool thank you. Closing issue as feature request is already under development.

BTW I am using hydra in two ways -

For rest API it is working like charm, and for SDK master branch will fix the issue which I am facing.
So if you want to quote example then you can use it.

@omry
Copy link
Collaborator

omry commented Dec 5, 2020

Cool thank you. Closing issue as feature request is already under development.

Actually this is developed already. It would be good if you try it out and provide any feedback.

About validating things like IP addresses etc:
There is an enhancement planned in OmegaConf [https://github.com/omry/omegaconf/issues/131] which should enable support for things like that.

@lalitpagaria
Copy link
Author

Actually this is developed already. It would be good if you try it out and provide any feedback.

Just now tested master branch and _convert_ fixed this issue.

Thanks for pointing about OmegaConf, I will check this out.

@omry
Copy link
Collaborator

omry commented Dec 5, 2020

Just now tested master branch and convert fixed this issue.

Great. try recursive instantiation too! :)

Thanks for pointing about OmegaConf, I will check this out.

Hydra is using OmegaConf for the config.
I suggest that you read the docs of OmegaConf (or check the slides in the readme).
It has many great features that you can already use (interpolation, missing values support and much more).

@lalitpagaria
Copy link
Author

@omry Is there possibility support _convert_ as parameter in compose function?
Idea to convert DictConfig to dict and ListConfig to list.

@omry
Copy link
Collaborator

omry commented Dec 6, 2020

You can use OmegaConf.to_container() directly.
Allow me to repeat the suggestion:
read the docs of OmegaConf.

@lalitpagaria
Copy link
Author

Thanks. My apologies, I was only focused on Hydra to achieve my configuration need.

@omry
Copy link
Collaborator

omry commented Dec 6, 2020

Hydra is utilizing OmegaConf extensively. It's a good use of your time to learn about OmegaConf as well.

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

No branches or pull requests

2 participants