-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove host_view and open_view from public API #1240
Conversation
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.
Thanks for the PR! I would like to see 2 small changes before merging, which I'll add myself on this PR in a follow-up:
- Rename
host_table()
andopen_table()
tohost()
andopen()
respectively. - Split out the autogenerated (docs) and unrelated lint-fix changes into a separate commit. I've done a poor job of keeping on top of both of these lately but I aim to fix this!
* Get a remote table handle from the Jupyter kernel, and | ||
* create a view on that handle to run Perspective in | ||
* distributed mode. | ||
*/ |
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 can't (AFAIK) enforce this in our lint rules, but please use //
exclusively for non-docstring comments.
Specifically, the /** .. */
format has a specific historical context of referring to a docstring, so this extra confusing to ex-java developers like myself!
@@ -31,6 +31,8 @@ For more information, see the | |||
* [.remove_delete(callback)](#module_perspective..view+remove_delete) | |||
* [~table](#module_perspective..table) | |||
* [new table()](#new_module_perspective..table_new) | |||
* [.get_index()](#module_perspective..table+get_index) | |||
* [.get_limit()](#module_perspective..table+get_limit) |
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.
Though I know I have not done this consistently in the past, I will make sure the docs are disted as part of the release cycle so we do not need these inter-mingled in our everyday review process.
eb08cee
to
9d84a4f
Compare
…sted/opened, update docs for distributed/server mode
e0da24f
to
436a600
Compare
Thanks for the PR! |
This PR removes
host_view
andopen_view
from the public API.With the improvements in the speed to create a flat view in #1235, the difference between using a
View
created on a server and creating aView
on the client using a handle to the server's table is minimal, if non-existent in most use-cases. This allows us to remove thehost_view
API, which doesn't fit in very well with our established public API semantics of "table as data container, view as query and data serializer."'host_view' shoe-horns in the ability to access a
View
over the network so that we can use it to create a table, and makes the explanations around distributed/server/client mode more complicated. Because the old context implementation added a small overhead (scaling linearly with the size of the underlying data),host_view
made sense for performance reasons.With the removal of
host_view
andopen_view
, Perspective's remote API only allows the hosting and opening ofTable
objects. This makes it far easier to reason about, especially when it comes to the distinctions between distributed, server, and client mode while maintaining the same feature set in the public API.Both distributed and server mode is easier to implement:
It is also trivial to host subsets of a table based on already-created views:
Changelog
host_view
andopen_view
from JS and Pythonopen_view
PerspectiveWidget.delete()
to delete all views created on widget's manager instance before deleting tableopen_table
and view creation