Repository navigation
Better and more explicit Pydantic error messages #2696
Description
Is your feature request related to a problem? Please describe.
The Pydantic errors can be very annoying to debug when the error messages are not explicit or (as often seen) completely missing. I provide two specific examples bellow but many more can be found throughout the codebase.
Describe the solution you'd like
Always include explicit error messages to all Pydating validation checks.
Additional context
A couple of examples:
>>> from feast import RepoConfig
>>> repo_config = RepoConfig(
... project="foo",
... )
Traceback (most recent call last):
File "my_script.py", line 2, in <module>
repo_config = RepoConfig(
File ".venv/lib/python3.9/site-packages/feast/repo_config.py", line 124, in __init__
super().__init__(**data)
File "pydantic/main.py", line 331, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for RepoConfig
__root__
(type=assertion_error)👆 In this example, the feast.errors.FeastProviderNotSetError exception should be raised instead of a blank AssertionError. The error message here would be explicit (i.e. "Provider is not set, but is required")
>>> from feast import RepoConfig
>>> repo_config = RepoConfig(
... project="foo",
... provider="local",
... offline_store=dict(
... type="my.custom.offline_store.CustomStore"
... ),
... )
Traceback (most recent call last):
File "my_script.py", line 2, in <module>
repo_config = RepoConfig(
File ".venv/lib/python3.9/site-packages/feast/repo_config.py", line 124, in __init__
super().__init__(**data)
File "pydantic/main.py", line 331, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for RepoConfig
__root__
(type=assertion_error)👆 Just like in the other example, it is impossible to see what the issue here is. In this case, the get_offline_config_from_type function should be raising an explicit exception with a "Offline store types should end with 'OfflineStore', got {offline_store_type} instead." message instead of an empty AssertionError. This way it would be immediately clear what the issue was.
Activity
achals commentedon May 16, 2022
Agreed @tpvasconcelos , we should definitely have better errors here. This should easy to do, we have a bunch of naked assert statements in
feast/repo_config.pyand just adding an error message there would hugely improve user experience.Would you be willing to take this on?
tpvasconcelos commentedon Aug 3, 2022
Automatically closed this by mistake. The merged PR was just an example implementation. I think more work needs to be done on this front.
@achals I don't have permission to reopen
stale commentedon Dec 16, 2022
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.