holoviz / panel

Panel: The powerful data exploration & web app framework for Python
https://panel.holoviz.org
BSD 3-Clause "New" or "Revised" License
4.78k stars 518 forks source link

Progress indicator inconsistencies #2685

Closed hyamanieu closed 3 years ago

hyamanieu commented 3 years ago

Panel: master ac04ed1c & 0.12 param: 1.10.2a13

The Progress indicator is a bit frustrating:

I would like to submit the following changes:

I'll submit a PR.

jbednar commented 3 years ago

Can you be more specific about the issues with None, with some reproducible code? The default is None and therefore allow_None should be true already. Is there some bug here being worked around?

hyamanieu commented 3 years ago

@jbednar please see here below. Yes it's a bug being worked around, and it seems to happen at bokeh level, not param now I'm taking a closer look to the stack trace further below.

browsers: firefox, chromium, brave os: Ubuntu 18.04 panel: 0.12.1 param: 1.11.1 bokeh: 2.3.2 python: 3.8

The bug only happens if the progress bar is displayed (either with display in a notebook like in my example or externally with progress.show() ). It also happens with the current master branch.

That bug or not, it's not possible to reset the bar in an indeterminate mode in the current branch anyways. It could be solved by using this.progressEl.removeAttribute("value") within progress.ts, but that won't fix the issue with "0" interpreted as a null by javascript. For this I couldn't find a fix. I would vote for not using None at all.

import panel as pn
pn.extension()

import param

print('panel version:', pn.__version__)

print('param version:', param.__version__)

progress = pn.widgets.Progress(value=None, bar_color='warning')
display(progress)

progress.value = 18

print('Is None allowed? ', progress.param.value.allow_None)

progress.value = -1

progress.value = 0

progress.value = None

Output:

panel version: 0.12.1
param version: 1.11.1

[bar]
Is None allowed?  True
--------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
/tmp/ipykernel_6553/797086022.py in <module>
     20 progress.value = 0
     21 
---> 22 progress.value = None

~/anaconda3/envs/dev38/lib/python3.8/site-packages/param/parameterized.py in _f(self, obj, val)
    314         instance_param = getattr(obj, '_instance__params', {}).get(self.name)
    315         if instance_param is not None and self is not instance_param:
--> 316             instance_param.__set__(obj, val)
    317             return
    318         return f(self, obj, val)

~/anaconda3/envs/dev38/lib/python3.8/site-packages/param/parameterized.py in _f(self, obj, val)
    316             instance_param.__set__(obj, val)
    317             return
--> 318         return f(self, obj, val)
    319     return _f
    320 

~/anaconda3/envs/dev38/lib/python3.8/site-packages/param/__init__.py in __set__(self, obj, val)
    622         If val is dynamic, initialize it as a generator.
    623         """
--> 624         super(Dynamic,self).__set__(obj,val)
    625 
    626         dynamic = callable(val)

~/anaconda3/envs/dev38/lib/python3.8/site-packages/param/parameterized.py in _f(self, obj, val)
    316             instance_param.__set__(obj, val)
    317             return
--> 318         return f(self, obj, val)
    319     return _f
    320 

~/anaconda3/envs/dev38/lib/python3.8/site-packages/param/parameterized.py in __set__(self, obj, val)
    982                       old=_old, new=val, type=None)
    983         for watcher in watchers:
--> 984             obj.param._call_watcher(watcher, event)
    985         if not obj.param._BATCH_WATCH:
    986             obj.param._batch_call_watchers()

~/anaconda3/envs/dev38/lib/python3.8/site-packages/param/parameterized.py in _call_watcher(self_, watcher, event)
   1643             event = self_._update_event_type(watcher, event, self_.self_or_cls.param._TRIGGER)
   1644             with batch_watch(self_.self_or_cls, enable=watcher.queued, run=False):
-> 1645                 self_._execute_watcher(watcher, (event,))
   1646 
   1647     def _batch_call_watchers(self_):

~/anaconda3/envs/dev38/lib/python3.8/site-packages/param/parameterized.py in _execute_watcher(self, watcher, events)
   1625             async_executor(partial(watcher.fn, *args, **kwargs))
   1626         else:
-> 1627             watcher.fn(*args, **kwargs)
   1628 
   1629     def _call_watcher(self_, watcher, event):

~/anaconda3/envs/dev38/lib/python3.8/site-packages/panel/reactive.py in _param_change(self, *events)
    250 
    251         for ref, (model, parent) in self._models.items():
--> 252             self._apply_update(events, msg, model, ref)
    253 
    254     def _process_events(self, events):

~/anaconda3/envs/dev38/lib/python3.8/site-packages/panel/reactive.py in _apply_update(self, events, msg, model, ref)
    204         if comm or not doc.session_context or state._unblocked(doc):
    205             with unlocked():
--> 206                 self._update_model(events, msg, root, model, doc, comm)
    207             if comm and 'embedded' not in root.tags:
    208                 push(doc, comm)

~/anaconda3/envs/dev38/lib/python3.8/site-packages/panel/reactive.py in _update_model(self, events, msg, root, model, doc, comm)
    217         ]
    218         try:
--> 219             model.update(**msg)
    220         finally:
    221             del self._changing[root.ref['id']]

~/anaconda3/envs/dev38/lib/python3.8/site-packages/bokeh/core/has_props.py in update(self, **kwargs)
    439         '''
    440         for k,v in kwargs.items():
--> 441             setattr(self, k, v)
    442 
    443     def update_from_json(self, json_attributes, models=None, setter=None):

~/anaconda3/envs/dev38/lib/python3.8/site-packages/bokeh/core/has_props.py in __setattr__(self, name, value)
    296 
    297         if name in props or (descriptor is not None and descriptor.fset is not None):
--> 298             super().__setattr__(name, value)
    299         else:
    300             matches, text = difflib.get_close_matches(name.lower(), props), "similar"

~/anaconda3/envs/dev38/lib/python3.8/site-packages/bokeh/core/property/descriptors.py in __set__(self, obj, value, setter)
    550             raise RuntimeError(f"{class_name}.{self.name} is a readonly property")
    551 
--> 552         self._internal_set(obj, value, setter=setter)
    553 
    554     def __delete__(self, obj):

~/anaconda3/envs/dev38/lib/python3.8/site-packages/bokeh/core/property/descriptors.py in _internal_set(self, obj, value, hint, setter)
    782 
    783         """
--> 784         value = self.property.prepare_value(obj, self.name, value)
    785         old = self._get(obj)
    786         self._real_set(obj, old, value, hint=hint, setter=setter)

~/anaconda3/envs/dev38/lib/python3.8/site-packages/bokeh/core/property/bases.py in prepare_value(self, owner, name, value)
    348         else:
    349             obj_repr = owner if isinstance(owner, HasProps) else owner.__name__
--> 350             raise ValueError(f"failed to validate {obj_repr}.{name}: {error}")
    351 
    352         if isinstance(owner, HasProps):

ValueError: failed to validate Progress(id='1040', ...).value: expected a value of type Integral, got None of type NoneType
mattpap commented 3 years ago

ValueError: failed to validate Progress(id='1040', ...).value: expected a value of type Integral, got None of type NoneType

Note Progress in this error message refers to panel.models.widgets.Progress, which defines its value (bokeh) property as a non-nullable integer. I suspect the discrepancy between this and the user facing abstraction layer (indicators), is a side effect of making bokeh's properties non-nullable by default in 2.3 (PR https://github.com/bokeh/bokeh/pull/10842). Perhaps Progress model was not updated since.

hyamanieu commented 3 years ago

Thanks matt, for the sake of fixing the bug I have modified the bokeh model but I would still vote for making it a deprecated type. cf commit https://github.com/holoviz/panel/pull/2686/commits/df33562e6f559a489219aa5344a0c7ef84df834c of PR #2686