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

exception cleanup #6347

Merged
merged 24 commits into from
Dec 20, 2022
Merged

exception cleanup #6347

merged 24 commits into from
Dec 20, 2022

Conversation

emmyoop
Copy link
Member

@emmyoop emmyoop commented Nov 30, 2022

resolves #6339

Description

Decorator to be added in #6413

This PR

  • converts all exceptions thrown by a function to directly raise an exception.
  • adds a few more type properties.
  • moves the exception functions used in jinja to a separate file in core/context
  • Organizes exception classes some
  • Updates most exceptions to take in parameters and build the exceptions in the class instead of just being passed a message

This PR does not

  • Convert every site of parent exceptions (ParsingException, CompilationException, etc) into a unique exception
  • Add any kind of logging
  • Add a decorator to the functions no longer used within core

Checklist

@cla-bot cla-bot bot added the cla:yes label Nov 30, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

1 similar comment
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

core/dbt/adapters/cache.py Outdated Show resolved Hide resolved
@emmyoop emmyoop force-pushed the er/ct-1567-exception-cleanup branch from e5b8c76 to a25bfb9 Compare December 5, 2022 20:13
@emmyoop emmyoop force-pushed the er/ct-1567-exception-cleanup branch from 9d19015 to 358746a Compare December 8, 2022 02:49
@@ -22,13 +22,20 @@
import pytz

from dbt.exceptions import (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we've hit the tipping point here where we should just import the dbt.exceptions module?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to do that other than not importing them individually?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some Python codebases have policies around that but we don't. I guess if the import list gets really long it might be easier to just import the package. If we did import dbt..exceptions as exc we could specify every one as exc.SomeException. I think some people prefer the specific imports because it makes it clearer which ones are used in this file. I'm kind of agnostic on the whole thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some people prefer the specific imports because it makes it clearer which ones are used in this file

I'm in this camp for sure.

core/dbt/exceptions.py Outdated Show resolved Hide resolved
@emmyoop emmyoop force-pushed the er/ct-1567-exception-cleanup branch from bd34660 to e85fb67 Compare December 15, 2022 14:34
@emmyoop emmyoop changed the title [WIP] exception cleanup exception cleanup Dec 15, 2022
@emmyoop emmyoop marked this pull request as ready for review December 15, 2022 21:41
@emmyoop emmyoop requested a review from a team as a code owner December 15, 2022 21:41
@emmyoop emmyoop requested a review from a team December 15, 2022 21:41
@emmyoop emmyoop requested review from a team as code owners December 15, 2022 21:41
@emmyoop emmyoop requested review from Fleid, aranke and gshank December 15, 2022 21:41
@@ -400,7 +400,7 @@ def safe_run_hooks(
thread_id="main",
timing=[],
message=f"{hook_type.value} failed, error:\n {exc.msg}",
adapter_response=exc.msg,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BaseResult was added here recently as part of #6059. My understanding is that the adapter response isn't important. Because i added types, this was failing since exc.msg is a string but adapter_response is a Dict[str, Any].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was some conversation recently about the adapter_response, but I'm pretty vague on what was discussed. I think it often doesn't actually have any content, but maybe should? But in any case assigning a string to it is clearly wrong.

Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a huge huge improvement over what we had before. It's so much easier to see all of the different exceptions that got hidden behind the generic functions.

I do confess to preferring the use of keyword arguments for constructing them, but wouldn't ask for that at this point.

Also ... Do you think it would be better to use dataclasses? The exceptions would then looks a lot more like the logging events, and we could do a standard "build the message" thing in a base class, kind of like we do for logs.

method_name="get_missing_columns",
arg_name="from_relation",
got_value=from_relation,
expected_type=self.Relation,
version="0.13.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of the "version" piece entirely, including in the message? We haven't supported version 0.13.0 in forever, so just a "this isn't supported" should be fine.

@@ -400,7 +400,7 @@ def safe_run_hooks(
thread_id="main",
timing=[],
message=f"{hook_type.value} failed, error:\n {exc.msg}",
adapter_response=exc.msg,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was some conversation recently about the adapter_response, but I'm pretty vague on what was discussed. I think it often doesn't actually have any content, but maybe should? But in any case assigning a string to it is clearly wrong.

Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but should we rename every exception to end in Error similar to Python's built-in exceptions?

At the very least, we should standardize on using either Error or Exception uniformly everywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1567] Clean up exceptions.py
4 participants