param icon indicating copy to clipboard operation
param copied to clipboard

Suppressing tracebacks for Param ValueErrors?

Open jbednar opened this issue 5 years ago • 10 comments

When a user sets a Parameter to a value that is not allowed, a ValueError is raised. Unless it is caught, a traceback is printed along with the ValueError, which is quite a bit of information:

image

All but the first couple of traceback lines are inside Param's own library, when in normal cases the problem is in the user's own code, so it would be nice to at least optionally suppress the extra traceback information. Does anyone know a clean way that this could be achieved so that our users aren't typically bombarded with all this useless detail about Param internals?

I can see various suggestions like setting sys.tracebacklimit or sys.excepthook (which doesn't seem to work with Jupyter), monkeypatching ipython (which doesn't work without Jupyter), using a contextmanager (which requires the user to do extra work every time they call Param code if they want to eliminate the traceback), or installing a jupyter extension (which is a Jupyter user's choice, not up to the library author).

Rather than any of those options, I was hoping for something that we as the authors of Param could put into Param that would specifically skip the Param-related part of the traceback, so that users get their usual full info for their own code's errors, but would get much more straightforward and direct info for the common case of a Parameter being set inappropriately. I know it's a lot to ask. :-)

jbednar avatar Aug 19 '20 21:08 jbednar

Depends if you are displaying this for "normal" users of param, or for the developers of param itself. I.e. whether you expect more users of param to be seeing errors in their code from mis-setting params, vs developers of param seeing errors in param ;)

Anyway, for users, what if I were to propose adding even more output? :)

Not using jupyter at the moment so here's the equivalent for me of your traceback:

$ python tmp.py 
Traceback (most recent call last):
  File "tmp.py", line 6, in <module>
    ParamClass(b="four")
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 2476, in __init__
    self.param._setup_params(**params)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 1046, in override_initialization
    fn(parameterized_instance, *args, **kw)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 1294, in _setup_params
    setattr(self, name, val)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 304, in _f
    F = f(self, obj, val)
  File "/home/sefkw/proj/param-fun/param/param/__init__.py", line 623, in __set__
    super(Dynamic,self).__set__(obj,val)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 304, in _f
    F = f(self, obj, val)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 873, in __set__
    self._validate(val)
  File "/home/sefkw/proj/param-fun/param/param/__init__.py", line 936, in _validate
    raise ValueError("Parameter '%s' must be an integer."%self.name)
ValueError: Parameter 'b' must be an integer.

For the example code

$ cat tmp.py
import param

class ParamClass(param.Parameterized):
    b = param.Integer(default=4)

ParamClass(b="four")

Making it longer...

$ python tmp.py 
Traceback (most recent call last):
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 2476, in __init__
    self.param._setup_params(**params)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 1046, in override_initialization
    fn(parameterized_instance, *args, **kw)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 1294, in _setup_params
    setattr(self, name, val)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 304, in _f
    F = f(self, obj, val)
  File "/home/sefkw/proj/param-fun/param/param/__init__.py", line 623, in __set__
    super(Dynamic,self).__set__(obj,val)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 304, in _f
    F = f(self, obj, val)
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 873, in __set__
    self._validate(val)
  File "/home/sefkw/proj/param-fun/param/param/__init__.py", line 936, in _validate
    raise ValueError("Parameter '%s' must be an integer."%self.name)
ValueError: Parameter 'b' must be an integer.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "tmp.py", line 6, in <module>
    ParamClass(b="four")
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 2478, in __init__
    raise ValueError(f"Error while creating {self.__class__}: {e.args[0]}") from e
ValueError: Error while instantiating <class '__main__.ParamClass'>: Parameter 'b' must be an integer.

This kind of thing became supported in python 3 (https://www.python.org/dev/peps/pep-3134/)

diff --git a/param/parameterized.py b/param/parameterized.py
index 696263c..0f7e954 100644
--- a/param/parameterized.py
+++ b/param/parameterized.py
@@ -2468,7 +2468,10 @@ class Parameterized(object):
         self._param_watchers = {}
 
         self.param._generate_name()
-        self.param._setup_params(**params)
+        try:
+            self.param._setup_params(**params)
+        except ValueError as e:
+           raise ValueError(f"Error while instantiating {self.__class__}: {e.args[0]}") from e
         object_count += 1
 
         # add watched dependencies

Note I also add the class (param owner) here. That kind of missing info always also used to annoy me.

~~(Just realized I meant "instantiating", not "creating" in above exception.)~~

Not saying the above would be the implementation, just giving quick example output. And would need to do the same kind of thing in other such places too (e.g. set).

ceball avatar Sep 13 '20 07:09 ceball

Depends if you are displaying this for "normal" users of param, or for the developers of param itself. I.e. whether you expect more users of param to be seeing errors in their code from mis-setting params, vs developers of param seeing errors in param ;)

I'm talking about "normal" users of param. There are many, many times more Parameterized classes in the world than the code in Param itself, and any time someone uses that code they can encounter such a ValueError. Errors within Param itself have got to be a very tiny fraction of such ValueErrors.

Your proposed extra info seems useful, but I would still say that everything before "ValueError: Parameter 'b' must be an integer." is useless and distracting. So if you want that extra info, what I'd want is probably:

$ python tmp.py 
ValueError: Parameter 'b' must be an integer.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "tmp.py", line 6, in <module>
    ParamClass(b="four")
  File "/home/sefkw/proj/param-fun/param/param/parameterized.py", line 2478, in __init__
    raise ValueError(f"Error while creating {self.__class__.__name__}: {e.args[0]}") from e
ValueError: Error while creating ParamClass: Parameter 'b' must be an integer.

That message links a line in the param code to a line in the user's code, which is the key to understanding what's going on here -- where the error was detected, and where it originated.

jbednar avatar Sep 13 '20 17:09 jbednar

(edit: now updated with code from above)

I'm talking about "normal" users of param.

Sorry, was joking about bugs in param/nobody using it :) I feel like I get to joke freely about param - maybe not.

If instead of doing

raise ValueError(f"Error while creating {self.__class__}: {e.args[0]}") from e

we did

raise ValueError(f"Error while creating {self.__class__}: {e.args[0]}") from None

we'd get the second traceback/extra info only.

$ python tmp.py 
Traceback (most recent call last):
  File "tmp.py", line 6, in <module>
    ParamClass(b="four")
  File "/home/sefkw/proj/param-gradual/param/param/parameterized.py", line 2475, in __init__
    raise ValueError(f"Error while instantiating {self.__class__}: {e.args[0]}") from None
ValueError: Error while instantiating <class '__main__.ParamClass'>: Parameter 'b' must be an integer.

Unlike my original suggestion, which I definitely think is an improvement (people read tracebacks bottom up to start with, I think), I'd have to think for a while how I feel about removing the original traceback completely.

ceball avatar Sep 13 '20 18:09 ceball

When I wrote this issue, I was specifically thinking about writing documentation, where I want to illustrate that Param can give feedback to the user about whether a value they supplied is correct. In that context, the extra traceback is very distracting; as you say, people have to figure out to just skip to the bottom anyway. In an interactive context maybe the extra layers of traceback are fine; maybe we can take a vote. But my vote is that as a user, I have not found them helpful. I'd find your latest version above very helpful.

jbednar avatar Sep 13 '20 20:09 jbednar

Maybe I'd be fine with this if it's controlled by a setting (possibly some existing setting that already controls something to do with the amount of stuff people see, e.g. related to verbosity or logging - not sure), and it could default to the short version.

ceball avatar Sep 13 '20 20:09 ceball

That sounds good to me. And I'd even want a non-default very terse setting, which would be useful for the docs (so that we can concisely illustrate that it detects all manner of wrong values).

jbednar avatar Sep 13 '20 20:09 jbednar

I think "add a setting that controls 'amount of traceback' from param validation errors" is a good first issue:

  • understandable problem
  • fix will be visible to many, who'll be grateful
  • there's a partial proof of concept (for instantiation of parameterized classes - but there are other places values get set, which would benefit from the same kind of summary error)
  • someone wants this feature enough to provide guidance/review
  • it's not an urgent/blocking feature (except in the way that all such generally ignored user experience/documentation features are urgent and extremely important...)
  • will result in an introduction to param internals, but without really having to change them

basic proof of concept

diff --git a/param/parameterized.py b/param/parameterized.py
index 696263c..0f7e954 100644
--- a/param/parameterized.py
+++ b/param/parameterized.py
@@ -2468,7 +2468,10 @@ class Parameterized(object):
         self._param_watchers = {}
 
         self.param._generate_name()
-        self.param._setup_params(**params)
+        try:
+            self.param._setup_params(**params)
+        except ValueError as e:
+           raise ValueError(f"Error while instantiating {self.__class__}: {e.args[0]}") from e
         object_count += 1
 
         # add watched dependencies

Replacing from e with from None to show only the second traceback.

extra: really short tracebacks

The extra request to optionally have a really short traceback, I don't want to think about the consequences ;) But

Screenshot from 2020-09-14 10-17-17

$ python tmp.py
ValueError: Error while instantiating <class '__main__.ParamClass'>: Parameter 'b' must be an integer.
diff --git a/param/parameterized.py b/param/parameterized.py
index 013802b..4815748 100644
--- a/param/parameterized.py
+++ b/param/parameterized.py
@@ -2453,7 +2453,22 @@ class Parameterized(object):
         self._param_watchers = {}
 
         self.param._generate_name()
-        self.param._setup_params(**params)
+        try:
+            self.param._setup_params(**params)
+        except ValueError as e:
+            newe = ValueError(f"Error while instantiating {self.__class__}: {e.args[0]}")
+            try:
+                ip = get_ipython()
+            except NameError:
+                ip = None
+            if ip:
+                import traceback
+                # see https://ipython.readthedocs.io/en/stable/config/integrating.html#custom-exception-tracebacks
+                newe._render_traceback_ = lambda: traceback.format_exc().splitlines()[-1::]
+                
+            sys.tracebacklimit = 0                
+            raise newe from None
+
         object_count += 1
 
         # add watched dependencies

in some re-usable form

ceball avatar Sep 14 '20 08:09 ceball

Thanks! I'm not sure if I qualify as a first-issuer, but I think that's enough detail that even I could implement it. :-) The main scope for original thought is then to consider all the cases where it would need to be applied, which I think is just __set__ and instantiation?

jbednar avatar Sep 15 '20 23:09 jbednar

sounds right, yes

ceball avatar Sep 17 '20 21:09 ceball

I would also appreciate this as an option (with short tracebacks by default).

jlstevens avatar Sep 23 '20 12:09 jlstevens