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
Trame support and new Jupyter backend #3385
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3385 +/- ##
==========================================
+ Coverage 94.13% 94.19% +0.05%
==========================================
Files 87 91 +4
Lines 19111 19581 +470
==========================================
+ Hits 17991 18444 +453
- Misses 1120 1137 +17 |
MNE integration test is failing as:
Trying to decide if I should implement the |
@banesullivan I tried this trame feature in pyvista and found a strange thing. I have a model with several data arrays, I want to want to change the data array and the data range of the model and scalar bar can be updated. First I remove the old scalar bar, then I add a new one. Here is the code for this part:
This can work for a simple pyvista application and also for the remote view of trame. But if I use the local view, the old scalar bar cannot be removed unless I refresh the page. |
This is most likely a little issue with the local view. I would think that if you update |
I was using |
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@larsoner, thanks for all of the feedback! I think I've addressed everything you brought up, but please double-check. Regarding the I really, really want to merge this today unless something significant comes up -- please let me know what you think. FYI, I noticed the UI menu bar issue in your screenshot and will try to sneak in a fix for that |
@jourdain, I feel like that would be best implemented in |
Should we just change the default mapper then if it detects that it's in a notebook with either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than one remaining bug found via my minimal testing, LGTM!
server_proxy_enabled=server_proxy_enabled, | ||
server_proxy_prefix=server_proxy_prefix, | ||
**kwargs, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, nice idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted this suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But didn't it look that way with ipyvtklink
already, suggesting that this maintains previous behavior?
Okay with me in principle for 100% -- 50% didn't seem to work all that well -- assuming there is some nice way in the public API to easily specify a fixed size. What is it with the current code? It seems like the current version completely ignores Plotter(..., window_size=...)
which seems unexpected/not backward compatible...
One way out of this might be to have Plotter(..., window_size='auto')
as a backward compat default which means "(600px, 600px)
for all backends except trame, which will use ('100%', '600px')
" or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larsoner, are you okay with me merging as is and following up in a separate PR (before the release) to expose a better mechanism for controlling the widget size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking here: #3879
Will try to address ASAP
We could... but I'd rather try to just fix this upstream and follow up with this sort of patch if necessary |
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
This reverts commit e798b85.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapproving. Really appreciate everyone's feedback here, and if there are any issues post-merge we'll be quick to fix them in follow-up PRs.
@banesullivan and @larsoner, let's merge this (after 24 ish hours green) and then follow-up with smaller PRs. I've had my fair share of 2,000+ LOC PRs and I think it will be easier to track issues and fix them in individual PRs. |
Exactly. This PR is touching a bunch of code at this point and it'll be much easier to track and review further changes as new PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
First thanks for this awesome new backend! But now a question: is the Trame backend available in the conda package? I get |
It should but it could be a WIP. I know that @banesullivan is working toward streamlining some of those conda recipes. |
The trame recipes just landed on conda-forge a few hours ago.
Should get you everything you need (just confirmed in a fresh env locally) |
Dear @banesullivan, thanks installation worked so far but I always get a 404 on the trame widget, e.g.:
I tried with and without |
Would you please open a new issue to track this |
This adds streamlined support for using PyVista with Trame and adds a new Trame-based Jupyter backend.
The new Trame-based backend is the backend that we will try to centralize around per #3690 and with it's addition comes an immediate deprecation of the
ipyvtklink
backend.Trame is a new framework for building lightweight, reactive web applications in a stateful manner. It has great support for VTK and thus PyVista. These changes add a
trame
module to PyVista that streamlines usage of thetrame-vtk
interface for use with a PyVistaPlotter
object.While the bulk of this PR is actually focused on the new Jupyter backend, this is also provided streamlined support for using PyVista and Trame to make reactive web apps. There are a few examples web apps included under:
examples_trame/
Basic Trame Web App
Screen.Recording.2022-09-29.at.12.01.39.PM.mov
Advanced Trame Web App
Screen.Recording.2023-01-21.at.12.12.57.PM.mov
New Jupyter Backend
Screen.Recording.2023-01-21.at.12.16.05.PM.mov
Jupyter Backend
See usage instructions in docs but simply
Additional Changes
This PR required a significant amount of work to PyVista and sneaks in a few changes that really could've been their own PRs. These include:
ren_win
torender_window
as a@property
suppress_rendering
@property
on theBasePlotter
that will suppress calls tovtkRenderWindow.Render()
for guarantee use of the PyVista plotter without a virtual frame buffer_id_name
attribute of the plotter so that it is a safe string to use as a variable name (added aP_
prefix)actors
@property
onBasePlotter
to get all actors on the Plotteradd_on_render_callback
. This has two modes (to register with actual VTK render events or be called explicitly by PyVista'sBasePlotter.render()
method (often to be paired withsuppress_rendering
)use_ipyvtk
argument andPYVISTA_USE_IPYVTK
environment variableResolves and Closes
This PR renders many issue non-actionable as the new Trame backend handles many issue with the other existing backends
pyvista
(pythreejs
oripygany
backend) incompatible withipywidgets>7.7.2.
#3274: since ipyvtklink is deprecated, this is no longer relevant or actionablesuppress_rendering
should fix this when combined with thepythreejs
backendplotter.show(return_viewer=True)
isn't updated when the plotter is #2940: this new Trame backend is the only backend that supports dynamic updatingWe will continue to address the remaining issues listed in #3690
Upstream work
A lot of upstream work was necessary for this new Jupyter backend and much thanks to @jourdain! Here are some (not all) of the relevant work:
Future Work