Better error handling: Re-raise with function reference on error #510

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

Jump to bottom

Open

gen-xu wants to merge 1 commit into ets-labs:master

from gen-xu:509-better-error-handling

Open

Better error handling: Re-raise with function reference on error #510

gen-xu wants to merge 1 commit into ets-labs:master from gen-xu:509-better-error-handling

Conversation

gen-xu

Copy link

Contributor

@gen-xu gen-xu commented Sep 14, 2021

This PR addresses #509

@gen-xu gen-xu force-pushed the 509-better-error-handling branch 3 times, most recently from d8a39d7 to 06b1470 Compare

September 14, 2021 20:20

@rmk135 rmk135 self-assigned this

Sep 14, 2021

@gen-xu gen-xu force-pushed the 509-better-error-handling branch from 06b1470 to 715480b Compare

September 14, 2021 20:24

@rmk135

Copy link

Member

rmk135 commented Sep 15, 2021

@gen-xu I can not accept the PR: with this change, we generally convert any type of exception to the Exception. This is a serious API change that can create unexpected problems. I'm thankful for the contribution, but we need to look for another solution to prettify the stack trace.

@gen-xu

Copy link

Contributor Author

gen-xu commented Sep 15, 2021 •

edited

Loading

@gen-xu I can not accept the PR: with this change, we generally convert any type of exception to the Exception. This is a serious API change that can create unexpected problems. I'm thankful for the contribution, but we need to look for another solution to prettify the stack trace.

I fully agree with your concern.

The reason why it is hard to debug on errors like that is mainly because cython doesn't add locals variables to the traceback object corresponding to the exception instance. see also: cython docs. So, even with good package like traceback_with_variables (which basically iterate over Exception.__traceback__.tb_frame.f_locals to print variables) we wouldn't be able to know the name of the callable object.

So I am wondering if adding locals manually to the e.__traceback__.tb_frame.f_locals would work in this case without breaking API.
I have added a new commit to the PR.

from dependency_injector.containers import DeclarativeContainer
from dependency_injector.providers import Callable, Configuration, Singleton
class A:
 def __init__(self, config) -> None:
 pass
class B:
 def __init__(self, config) -> None:
 pass
class SomeContainer(DeclarativeContainer):
 config = Configuration()
 a = Singleton(A, config)
 b = Singleton(B)
 c = Callable(print, a, b)

For the code example above, after this change, the new stacktrace would be something like this

s = SomeContainer()
s.c()

After

Before

Traceback (most recent call last):
 File "C:\Users\Gen\Repos\python-dependency-injector\exp.py", line 20, in <module>
 s.c()
 File "src/dependency_injector/providers.pxd", line 579, in dependency_injector.providers.__call
 return call(*args, **kwargs)
TypeError: __init__() missing 1 required positional argument: 'config'

Traceback (most recent call last):
 File "C:\Users\Gen\Repos\python-dependency-injector\exp.py", line 20, in <module>
 s.c()
 File "src/dependency_injector/providers.pyx", line 207, in dependency_injector.providers.Provider.__call__
 result = self._provide(args, kwargs)
 File "src/dependency_injector/providers.pyx", line 1212, in dependency_injector.providers.Callable._provide
 return __callable_call(self, args, kwargs)
 File "src/dependency_injector/providers.pxd", line 606, in dependency_injector.providers.__callable_call
 
 File "src/dependency_injector/providers.pxd", line 547, in dependency_injector.providers.__call
 args = __provide_positional_args(
 File "src/dependency_injector/providers.pxd", line 390, in dependency_injector.providers.__provide_positional_args
 value = __get_value(injection)
 File "src/dependency_injector/providers.pxd", line 345, in dependency_injector.providers.__get_value
 return self.__value()
 File "src/dependency_injector/providers.pyx", line 207, in dependency_injector.providers.Provider.__call__
 result = self._provide(args, kwargs)
 File "src/dependency_injector/providers.pyx", line 2822, in dependency_injector.providers.Singleton._provide
 instance = __factory_call(self.__instantiator, args, kwargs)
 File "src/dependency_injector/providers.pxd", line 620, in dependency_injector.providers.__factory_call
 cdef inline object __factory_call(Factory self, tuple args, dict kwargs):
 File "src/dependency_injector/providers.pxd", line 606, in dependency_injector.providers.__callable_call
 File "src/dependency_injector/providers.pxd", line 579, in dependency_injector.providers.__call
 return call(*args, **kwargs)
TypeError: __init__() missing 1 required positional argument: 'config'

and with the help of traceback_with_variables

s = SomeContainer()
try:
 s.c()
except:
 from traceback_with_variables import print_exc
 print_exc()

After

Before

Traceback with variables (most recent call last):
 File "C:\Users\Gen\Repos\python-dependency-injector\exp.py", line 21, in <module>
 s.c()
 __name__ = '__main__'
 __doc__ = None
 __package__ = None
 __loader__ = <_frozen_importlib_external.SourceFileLoader object at 0x00000248EF47A100>
 __spec__ = None
 __annotations__ = {}
 __builtins__ = <module 'builtins' (built-in)>
 __file__ = 'C:\\Users\\Gen\\Repos\\python-dependency-injector\\exp.py'
 __cached__ = None
 DeclarativeContainer = <class 'dependency_injector.containers.DeclarativeContainer'>
 Callable = <class 'dependency_injector.providers.Callable'>
 Configuration = <class 'dependency_injector.providers.Configuration'>
 Singleton = <class 'dependency_injector.providers.Singleton'>
 print_exc = <function print_exc at 0x00000248F2254EE0>
 A = <class '__main__.A'>
 B = <class '__main__.B'>
 SomeContainer = <class '__main__.SomeContainer'>
 s = <dependency_injector.containers.DynamicContainer object at 0x00000248EF9C8FA0>
 File "src/dependency_injector/providers.pxd", line 579, in dependency_injector.providers.__call
 return call(*args, **kwargs)
 args = ()
 call = <class '__main__.B'>
 context_args = ()
 context_kwargs = {}
 e = TypeError("__init__() missing 1 required positional argument: 'config'")
 injection_args = ()
 injection_args_len = 0
 injection_kwargs = ()
 injection_kwargs_len = 0
 is_future_args = False
 is_future_kwargs = False
 kwargs = {}
builtins.TypeError: __init__() missing 1 required positional argument: 'config'

Traceback with variables (most recent call last):
 File "src/dependency_injector/providers.pxd", line 579, in dependency_injector.providers.__call
 return call(*args, **kwargs)
 __name__ = 'dependency_injector.providers'
 __doc__ = 'Providers module.'
 __package__ = 'dependency_injector'
 __loader__ = <_frozen_importlib_external.ExtensionFileLoader object at 0x000001D78BFA9460>
 __spec__ = ModuleSpec(name='dependency_injector.providers', loader=<_frozen_importlib_external.ExtensionFileLoader object at 0x000001D78BFA9460>, origin='C:\\Users\\Gen\\AppData\\Local\\Programs\\Python\\Python39\\lib\\site-packages\\dependency_injector\\providers.cp39-win_amd64.pyd')
 ...
 Provider = <class 'dependency_injector.providers.Provider'>
 Object = <class 'dependency_injector.providers.Object'>
 Self = <class 'dependency_injector.providers.Self'>
 Delegate = <class 'dependency_injector.providers.Delegate'>
 Dependency = <class 'dependency_injector.providers.Dependency'>
 ExternalDependency = <class 'dependency_injector.providers.ExternalDependency'>
 ...
 Error = <class 'dependency_injector.errors.Error'>
 NoSuchProviderError = <class 'dependency_injector.errors.NoSuchProviderError'>
 config_env_marker_pattern = re.compile('\\${(?P<name>[^}^{:]+)(?P<separator>:?)(?P<default>.*?)}')
 _resolve_config_env_markers = <built-in function _resolve_config_env_markers>
 _parse_ini_file = <built-in function _parse_ini_file>
 YamlLoader = <class 'dependency_injector.providers.YamlLoader'>
 UNDEFINED = <object object at 0x000001D78B518F00>
 CHILD_PROVIDERS = (<class 'dependency_injector.providers.Dependency'>, <class 'dependency_injector.providers.DependenciesContainer'>, <class 'dependency_injector.providers.Container'>)
 __add_sys_streams = <built-in function __add_sys_streams>
 merge_dicts = <built-in function merge_dicts>
 traverse = <built-in function traverse>
 isawaitable = <built-in function isawaitable>
 iscoroutinefunction = <built-in function iscoroutinefunction>
 isasyncgenfunction = <built-in function isasyncgenfunction>
 __pyx_unpickle_Provider = <built-in function __pyx_unpickle_Provider>
 __pyx_unpickle_Object = <built-in function __pyx_unpickle_Object>
 __pyx_unpickle_Self = <built-in function __pyx_unpickle_Self>
 ...
 __test__ = {}
builtins.TypeError: __init__() missing 1 required positional argument: 'config'

@gen-xu gen-xu force-pushed the 509-better-error-handling branch 2 times, most recently from 9abd18a to f971109 Compare

September 15, 2021 10:14

@gen-xu


 Add locals on callable exception

9273ab3

@gen-xu gen-xu force-pushed the 509-better-error-handling branch from f971109 to 9273ab3 Compare

September 15, 2021 10:15

@whysage whysage mentioned this pull request

Nov 1, 2021

Better error handling #509 #528

Open

@rmk135

Copy link

Member

rmk135 commented Nov 8, 2021

I have added a new commit to the PR.

@gen-xu my apologies for the super delayed feedback. Thanks a lot for working on this.

I think we provide too much noise if we go that way:

Traceback with variables (most recent call last):
 File "C:\Users\Gen\Repos\python-dependency-injector\exp.py", line 21, in <module>
 s.c()
 __name__ = '__main__'
 __doc__ = None
 __package__ = None
 __loader__ = <_frozen_importlib_external.SourceFileLoader object at 0x00000248EF47A100>
 __spec__ = None
 __annotations__ = {}
 __builtins__ = <module 'builtins' (built-in)>
 __file__ = 'C:\\Users\\Gen\\Repos\\python-dependency-injector\\exp.py'
 __cached__ = None
 DeclarativeContainer = <class 'dependency_injector.containers.DeclarativeContainer'>
 Callable = <class 'dependency_injector.providers.Callable'>
 Configuration = <class 'dependency_injector.providers.Configuration'>
 Singleton = <class 'dependency_injector.providers.Singleton'>
 print_exc = <function print_exc at 0x00000248F2254EE0>
 A = <class '__main__.A'>
 B = <class '__main__.B'>
 SomeContainer = <class '__main__.SomeContainer'>
 s = <dependency_injector.containers.DynamicContainer object at 0x00000248EF9C8FA0>

Could you review #528 ? What do you think if we go that way?

@whysage

Copy link

Contributor

whysage commented Dec 5, 2021

Hi!
@rmk135 @gen-xu
addition ping for updates)

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Better error handling: Re-raise with function reference on error #510

Are you sure you want to change the base?

Better error handling: Re-raise with function reference on error #510

Uh oh!

Conversation

@gen-xu gen-xu commented Sep 14, 2021

Uh oh!

rmk135 commented Sep 15, 2021

Uh oh!

gen-xu commented Sep 15, 2021 •

edited

Loading

Uh oh!

Uh oh!

rmk135 commented Nov 8, 2021

Uh oh!

whysage commented Dec 5, 2021

Uh oh!

Uh oh!

Uh oh!

Better error handling: Re-raise with function reference on error #510

Are you sure you want to change the base?

Better error handling: Re-raise with function reference on error #510

Uh oh!

Conversation

@gen-xu gen-xu commented Sep 14, 2021

Uh oh!

rmk135 commented Sep 15, 2021

Uh oh!

gen-xu commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmk135 commented Nov 8, 2021

Uh oh!

whysage commented Dec 5, 2021

Uh oh!

Uh oh!

gen-xu commented Sep 15, 2021 •

edited

Loading