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

Better and more explicit Pydantic error messages #2696

Closed
tpvasconcelos opened this issue May 15, 2022 · 3 comments · Fixed by #2708
Closed

Better and more explicit Pydantic error messages #2696

tpvasconcelos opened this issue May 15, 2022 · 3 comments · Fixed by #2708
Labels
Community Contribution Needed We want community to contribute critical good first issue Good for newcomers kind/bug priority/p2 wontfix This will not be worked on

Comments

@tpvasconcelos
Copy link
Contributor

tpvasconcelos commented May 15, 2022

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.

@tpvasconcelos tpvasconcelos added the kind/feature New feature or request label May 15, 2022
@achals achals added good first issue Good for newcomers kind/bug priority/p2 and removed kind/feature New feature or request labels May 16, 2022
@achals
Copy link
Member

achals commented 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.py and just adding an error message there would hugely improve user experience.

Would you be willing to take this on?

@tpvasconcelos
Copy link
Contributor Author

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

@adchia adchia reopened this Aug 3, 2022
@kevjumba kevjumba added Community Contribution Needed We want community to contribute critical labels Aug 3, 2022
@stale
Copy link

stale bot commented 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.

@stale stale bot added the wontfix This will not be worked on label Dec 16, 2022
@stale stale bot closed this as completed Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Needed We want community to contribute critical good first issue Good for newcomers kind/bug priority/p2 wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants