Skip to content

Log figure render count and start/end #13503

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

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Conversation

droumis
Copy link
Member

@droumis droumis commented Nov 1, 2023

This branch, created by @ianthomas23 (and now rebased onto branch-3.4), addresses the need for a console log render count to measure the latency of display and interaction updates. It includes the figure id, count, and state (start, end).

As this is likely not ideal to have printed for every user, the next step is to limit the conditions under which it is emitted. I could use feedback/guidance on achieving this, maybe with an environment variable like 'BOKEH_RENDER_COUNT'.

UPDATE: I've put the console messages behind Bokeh log level "trace" as suggested.

@bryevdv
Copy link
Member

bryevdv commented Nov 1, 2023

This should use logger from core/logging and probably at level "trace" e.g. like

logger.trace("WebGL is supported, but not the required extensions")

then BOKEH_LOG_LEVEL=trace can be used to enable.

Edit: I guess the risk here is that trace may already spit out voluminous output and maybe that's hard to deal with or even distorts performance all on its own. But let's start with this approach and see how it goes.

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a8fb295) 92.54% compared to head (8886ac7) 92.54%.
Report is 10 commits behind head on branch-3.4.

Additional details and impacted files
@@              Coverage Diff               @@
##           branch-3.4   #13503      +/-   ##
==============================================
- Coverage       92.54%   92.54%   -0.01%     
==============================================
  Files             323      323              
  Lines           20493    20493              
==============================================
- Hits            18966    18965       -1     
- Misses           1527     1528       +1     

@mattpap
Copy link
Contributor

mattpap commented Nov 7, 2023

To note, a lot of information can be extracted from browsers' devtools, e.g.:
image
Such profile can be saved as JSON and later processed and evaluated. They can also be collected when running bokehjs' tests, because we use devtools' API there, which exposes all of the features available in the UI. We already use this API to collect general metrics regarding memory usage, DOM node and event listener count, etc., which can be then visualized similarly to test reports (http://localhost:5777/integration/metrics).

@bryevdv
Copy link
Member

bryevdv commented Nov 7, 2023

Such profile can be saved as JSON and later processed and evaluated.

Is the format stable across browser versions? Or even browsers? Resolving this with docs updates and some possibly some helper scripts under scripts sounds ideal, but I'm also not sure what the actual requirements are or whether the devtools logs would be sufficient for them. cc @droumis @philippjfr

@droumis
Copy link
Member Author

droumis commented Dec 6, 2023

Thanks for the patience. To provide more context about what we are trying to achieve:

Summary

We are developing several workflows (HoloViz and Bokeh code), each demonstrating how to build a certain type of visualization relevant to biomedical research (e.g. Bokeh's subcoordinates_zoom EEG example is inspired by one of the workflows)

For each type of workflow, we need to benchmark the latency of the initial display, as well as the latency of the updated display following an interaction event (e.g. zoom), as a function of dataset size and backend (canvas, WebGL).

Importantly, we need to be able to compare the latency measurements for the same workflow, given different versions of the workflow code (comparing script version A vs B) or changes to the underlying libraries (comparing branch A vs B).

  • e.g. benchmark diagram

Less importantly, but in the longer term, we would like to have the workflow benchmarks used as an automated test for candidate library releases (ASV style).


What has been done so far?

Ian created this PR's log_render_count branch to emit figure id, count, and state (start, end) to the browser console. He also set up a system with ASV and Playwright to run repeated and parameterized (data size and backend) displays and interactions from simple HoloViz/Bokeh code snippets and use the messages from the console to measure latency.

PRs:

@droumis
Copy link
Member Author

droumis commented Dec 20, 2023

Aside from the unrelated CI issue (which is failing on all PRs right now), this should be ready for review @mattpap or @bryevdv. thanks!

@droumis droumis added type: feature grant: CZI R5 Funded by CZI Round 5 grant and removed status: WIP labels Dec 20, 2023
@bryevdv
Copy link
Member

bryevdv commented Dec 20, 2023

I don't necessarily have any problem with adding this behind trace if it is blocking downstream work, but I thought the plan was to first investigate whether the necessary information could be obtained with built-in browser dev tools. Was there any result or conclusion reached regarding that that I missed?

@droumis
Copy link
Member Author

droumis commented Dec 20, 2023

Hi @bryevdv, since the approach we've constructed utilizes ASV and Playwright to listen for these console messages on the fly and execute parameterized runs with different backends and data amounts interleaved, it would indeed unblock our progress if this PR could be merged and included in the upcoming release, as long as there's no harm in the implementation. We don't have the availability to dive into the devtools approach anytime soon and evaluate its sufficiency, but maybe in the future it might be reasonable thing to do it as a cross-validation of measurements. I think this is a pretty unique case - wanting to get the timing of the paint - so I don't anticipate that this would lead to many more trace-based measurement PRs.

@mattpap
Copy link
Contributor

mattpap commented Dec 20, 2023

I don't mind merging this. console.trace() doesn't affect performance when trace-level logging isn't enabled, so this only affects bundle size. I meant to add support for stripping debug code (like recently infamous assert_debug()) from bundles for a long while, so eventually this also won't be an issue as well.

Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with @mattpap's one suggested change

@droumis droumis force-pushed the ianthomas23/log_render_count branch from 509901d to 8886ac7 Compare December 26, 2023 18:50
@droumis
Copy link
Member Author

droumis commented Dec 26, 2023

Thanks! The suggested change is pushed

@droumis
Copy link
Member Author

droumis commented Jan 9, 2024

this can be merged now

@bryevdv bryevdv added this to the 3.4 milestone Jan 9, 2024
@bryevdv bryevdv merged commit 59751d9 into branch-3.4 Jan 9, 2024
@bryevdv bryevdv deleted the ianthomas23/log_render_count branch January 9, 2024 19:27
@bryevdv bryevdv modified the milestones: 3.4, 3.3.4 Jan 24, 2024
bryevdv pushed a commit that referenced this pull request Jan 24, 2024
* Log PlotView render count

* Include model id in render console messages

* remove new code annotations

* test logger trace

* implement log level trace handling

* remove extra semicolons

apply suggested message

* apply suggested message

---------

Co-authored-by: Ian Thomas <ianthomas23@gmail.com>
@bryevdv bryevdv mentioned this pull request Jan 24, 2024
13 tasks
bryevdv added a commit that referenced this pull request Jan 25, 2024
* Log figure render count and start/end (#13503)

* Log PlotView render count

* Include model id in render console messages

* remove new code annotations

* test logger trace

* implement log level trace handling

* remove extra semicolons

apply suggested message

* apply suggested message

---------

Co-authored-by: Ian Thomas <ianthomas23@gmail.com>

* Update CoC to use reporting form (#13647)

* Let client ping failure clean up session (#13655)

* Let client ping failure clean up session

* Fix typo

* Add test

* Fix codestyle issues

* Ignore pandas' deprecation warning regarding pyarrow (#13657)

* Ignore pandas' deprecation warning regarding pyarrow

* Fix other deprecation warnings

* Support pandas 1.x and 2.x/3.x simultaneously

* Fix missing User Guide sidebars (#13659)

* Update environment-release-build.yml

* Use pip syntax

* Force import line wrap by count and line length (#13626)

* Make line_length cutoff consistent at 88

* Update the codebase

* Use relative imports to shorten line lengths

* add rst-files for glyphes (#13597)

* Update app.rst (#13588)

* Add Metadata for airports_graph.py and geojson_source.py (#13568)

* Added metadata to airports_graph.py

* Added metadata to geojson_source.py

* Remove trailing whitespaces

* Fix formating of metadata, remove some apis from airports_graph.py

* Add space between tripple qoute and first letter

* remove trailing white space

* Add Iterable kind and use it in BooleanFilter.booleans (#13661)

* update switcher.json and release notes

* baseline test manual fixup

---------

Co-authored-by: Demetris Roumis <roumis.d@gmail.com>
Co-authored-by: Ian Thomas <ianthomas23@gmail.com>
Co-authored-by: Pavithra Eswaramoorthy <pavithraes@outlook.com>
Co-authored-by: Philipp Rudiger <prudiger@anaconda.com>
Co-authored-by: Mateusz Paprocki <mattpap@gmail.com>
Co-authored-by: Moritz Schreiber <68053396+mosc9575@users.noreply.github.com>
Co-authored-by: Icoti <94801369+Icoti@users.noreply.github.com>
Co-authored-by: dasha-moskv <81783162+dasha-moskv@users.noreply.github.com>
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
* Log PlotView render count

* Include model id in render console messages

* remove new code annotations

* test logger trace

* implement log level trace handling

* remove extra semicolons

apply suggested message

* apply suggested message

---------

Co-authored-by: Ian Thomas <ianthomas23@gmail.com>
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
@droumis droumis self-assigned this Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
grant: CZI R5 Funded by CZI Round 5 grant status: accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Assist in measuring display and interaction latency
4 participants