-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Remove host_view and open_view from public API #1240
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
Conversation
texodus
left a comment
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!
| * [~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_viewandopen_viewfrom the public API.With the improvements in the speed to create a flat view in #1235, the difference between using a
Viewcreated on a server and creating aViewon 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_viewAPI, 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
Viewover 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_viewmade sense for performance reasons.With the removal of
host_viewandopen_view, Perspective's remote API only allows the hosting and opening ofTableobjects. 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_viewandopen_viewfrom JS and Pythonopen_viewPerspectiveWidget.delete()to delete all views created on widget's manager instance before deleting tableopen_tableand view creation