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

Deprecate non-Trame Jupyter backends #3902

Merged
merged 4 commits into from
Jan 31, 2023
Merged

Deprecate non-Trame Jupyter backends #3902

merged 4 commits into from
Jan 31, 2023

Conversation

banesullivan
Copy link
Member

The final step to resolve #3690 -- deprecates all previous notebook backends in favor of the new Trame-based backend.

@banesullivan banesullivan added this to the v0.38 milestone Jan 30, 2023
@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Jan 30, 2023
@banesullivan
Copy link
Member Author

@pyvista/developers, I'd like to land this for 0.38 but if there are reservations about deprecating all other backends so quickly after the introduction of the new Trame-based backend, we can delay this to 0.39

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #3902 (67588cb) into main (6884399) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3902   +/-   ##
=======================================
  Coverage   94.19%   94.20%           
=======================================
  Files          91       91           
  Lines       19583    19632   +49     
=======================================
+ Hits        18447    18494   +47     
- Misses       1136     1138    +2     

@larsoner
Copy link
Contributor

I think we need #3879 first, and maybe also the code to tell people nicely when a plot isn't expected to work (volume rendering or whatever?)

@banesullivan
Copy link
Member Author

I think we need #3879 first,

On its way, but I need land #3889 and #3897 first. Do you think you could help review those?

maybe also the code to tell people nicely when a plot isn't expected to work (volume rendering or whatever?)

At this point, the trame-client side backend is better supported and more feature rich than any of the other backends. There are no longer any features (that I'm aware of) that are support by other backends and not the trame backend.

FYI, volume rendering works fine now! (ref Kitware/trame-vtk#19)

akaszynski
akaszynski previously approved these changes Jan 30, 2023
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

I'm happy to change the backend default, especially now that volume rendering works by default.

We can always submit a patch release should we need to revert.

@akaszynski
Copy link
Member

Fairly sure that warnings from the docbuild are difficult to suppress: https://github.com/sphinx-doc/sphinx/pull/11024/files

@banesullivan
Copy link
Member Author

Erm... Should I just remove where these backends are used in examples?

@akaszynski
Copy link
Member

Erm... Should I just remove where these backends are used in examples?

I'd advocate for a soft deprecation here. If we want to, we can include a note or warning directive to indicate that these are being replaced. This will give users a chance to see and compare the new vs. the old.

@akaszynski
Copy link
Member

Ironically, the brand new jupyter backend doesn't support the coolest feature we have: being able to visualize in static webpages.

I'd really like to make sure we don't block the usage of these alternative backends until we can be feature parity.

@banesullivan
Copy link
Member Author

Ironically, the brand new jupyter backend doesn't support the coolest feature we have: being able to visualize in static webpages.

I'd really like to make sure we don't block the usage of these alternative backends until we can be feature parity.

We'll have the static output from trame in the next couple weeks... we could just land this PR after 0.38 so that the deprecation comes with that final feature

@akaszynski
Copy link
Member

We'll have the static output from trame in the next couple weeks... we could just land this PR after 0.38 so that the deprecation comes with that final feature

Awesome.

@banesullivan
Copy link
Member Author

with #3909 and soon-to-come static embedding, what do you think @akaszynski? Merge for 0.38 or wait for 0.39?

@akaszynski
Copy link
Member

with #3909 and soon-to-come static embedding, what do you think @akaszynski? Merge for 0.38 or wait for 0.39?

I'm good with merging this now. Users can still use the older backends, even without warnings since we suppress those.

@banesullivan banesullivan merged commit 348bff8 into main Jan 31, 2023
@banesullivan banesullivan deleted the maint/trame-wins branch January 31, 2023 17:31
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Towards a single, PyVista/VTK-native Jupyter Backend
3 participants