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

factor out error handling logic & re-use in FunctionHandler #9832

Merged
merged 5 commits into from Mar 29, 2020

Conversation

@wearpants
Copy link
Contributor

wearpants commented Mar 26, 2020

  • issues: fixes #9831
  • tests added / passed
  • release document entry (if new feature or API change)
@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Mar 26, 2020

Hi @wearpants Thanks for the PR, there is currently just a small codebase check for isort import order failing. Here is a diff that will rectify:

 # Bokeh imports
 from ...util.callback_manager import _check_callback
+from .code_runner import _handle_exception
 from .handler import Handler
-from .code_runner import _handle_exception

I'll leave a few other comments inline

filename, line_number, func, txt = traceback.extract_tb(exc_traceback)[-1]

handler._error = "%s\nFile \"%s\", line %d, in %s:\n%s" % (str(e), os.path.basename(filename), line_number, func, txt)

This comment has been minimized.

Copy link
@bryevdv

bryevdv Mar 26, 2020

Member

This seems pretty independent of CodeRunner. Perhaps this would be better in handler.py ? (with the doctring updated)

This comment has been minimized.

Copy link
@wearpants

wearpants Mar 27, 2020

Author Contributor

sounds fine

self._func(doc)
except Exception as e:
_handle_exception(self, e)
finally:

This comment has been minimized.

Copy link
@bryevdv

bryevdv Mar 26, 2020

Member

Not sure this finally is really necessary in a practical sense. If there was an exception in the function, nothing is going to work anyway.

This comment has been minimized.

Copy link
@wearpants

wearpants Mar 27, 2020

Author Contributor

just trying to be consistent, the code using safe_to_fork says "don't run if any changes have been made", but we have no way of knowing what user code did before errorring.

@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Mar 26, 2020

@wearpants My one concern here is: how does this perform in Jupyter notebooks? When embedding a Bokeh app in a notebook, FunctionHandler is the usual method, and AFAIK currently the exception will leak out to the output cell for the user to see. Can you confirm the behavior in notebooks?

@wearpants

This comment has been minimized.

Copy link
Contributor Author

wearpants commented Mar 27, 2020

re: Jupyter - will check - is there an example you can point me to (I'm coming from holoviz/panel, I haven't used bare bokeh much). As written, the error should just get logged & swallowed - how do we want to handle turning traps on in Django, but off in Notebooks? Pass a flag here maybe: https://github.com/bokeh/bokeh/blob/master/bokeh/server/django/routing.py#L71 ?

Thanks for the prompt review!

@mattpap mattpap added the status: WIP label Mar 27, 2020
@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Mar 27, 2020

@wearpants

This comment has been minimized.

Copy link
Contributor Author

wearpants commented Mar 28, 2020

Ok, think I addressed everything here, though @bryevdv if you can confirm the notebook behaves as you like that would be good.

Can we get this in a 2.0.1 release? Maintaining a custom package is a bit painful - this is one complex build!

@wearpants wearpants requested a review from bryevdv Mar 28, 2020
@bryevdv

This comment has been minimized.

Copy link
Member

bryevdv commented Mar 29, 2020

Thanks @wearpants !

@bryevdv bryevdv merged commit cbe9cfb into bokeh:master Mar 29, 2020
19 checks passed
19 checks passed
build
Details
codebase
Details
examples
Details
js-test
Details
integration-tests
Details
unit-test (ubuntu-latest, 3.6)
Details
unit-test (ubuntu-latest, 3.7)
Details
unit-test (ubuntu-latest, 3.8)
Details
unit-test (macos-latest, 3.6)
Details
unit-test (macos-latest, 3.7)
Details
unit-test (macos-latest, 3.8)
Details
unit-test (windows-latest, 3.6)
Details
unit-test (windows-latest, 3.7)
Details
unit-test (windows-latest, 3.8)
Details
minimal-deps
Details
documentation
Details
downstream
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.