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 error handling: Re-raise with function reference on error #510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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

@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 gen-xu force-pushed the 509-better-error-handling branch from f971109 to 9273ab3 Compare September 15, 2021 10:15
@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)

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

Successfully merging this pull request may close these issues.

3 participants