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

Remove host_view and open_view from public API #1240

Merged
merged 4 commits into from
Dec 16, 2020
Merged

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Oct 29, 2020

This PR removes host_view and open_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 a View 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 the host_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 and open_view, Perspective's remote API only allows the hosting and opening of Table 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:

manager = PerspectiveManager()
manager.host_table("data_source", TABLE)
const worker = perspective.worker();
const websocket = perspective.websocket(URL);

// Open the table on the server - this is all that is needed for server mode
const remote_table = websocket.open_table("data_source");

// For distributed mode, create a view
const view = remote_table.view();

// Pass it into the JS table constructor
const client_table = worker.table(view);

It is also trivial to host subsets of a table based on already-created views:

# Table with large dataset that might be too big to transmit in one go
table = perspective.Table(big_dataset)

# Create a view on a subset of the data
filtered_view = table.view(filter=[["Sales", ">", 100]])

# Create a table using the filtered view
filtered_table = perspective.Table(filtered_view.to_arrow())

# Add an `on_update` callback to keep the tables in sync
def updater(port_id, delta):
    filtered_table.update(delta)

filtered_view.on_update(updater, mode="row")

# Easy to host the tables
manager.host_table("data_source", table)
manager.host_table("data_source_filtered", filtered_table)

Changelog

  • Remove host_view and open_view from JS and Python
  • Refactor Jupyterlab widget to create a new view instead of using open_view
  • Fix regression in PerspectiveWidget.delete()to delete all views created on widget's manager instance before deleting table
  • Update examples to use open_table and view creation
  • Update documentation to include distributed/server mode in Python docs

Copy link
Member

@texodus texodus left a 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() and open_table() to host() and open() 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.
*/
Copy link
Member

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)
Copy link
Member

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.

@sc1f sc1f force-pushed the remove-host-view branch from eb08cee to 9d84a4f Compare November 5, 2020 17:06
@texodus
Copy link
Member

texodus commented Dec 16, 2020

Thanks for the PR!

@texodus texodus merged commit 2c51cc2 into master Dec 16, 2020
@texodus texodus deleted the remove-host-view branch December 16, 2020 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants