pydeck and @deck.gl/jupyter-widget: Add support for dynamic deck.gl custom Layer registration #4233

ajduberstein · 2020-02-04T23:43:18Z

Background

Change List

Load custom scripts using XMLHttpRequest (works in Notebook, JupyterLab, and pure HTML environments)
Layer registration

coveralls · 2020-02-04T23:56:49Z

Coverage decreased (-0.2%) to 80.701% when pulling 4c98bd0 on ajd/custom-layer-loader into 3b45d23 on master.

ajduberstein · 2020-02-06T03:26:30Z

This PR is currently working with the code from this repo, which produces the bundle that I'm hosting here.

ibgreen

@ajdubersteing This looks great, thank you very much for putting this in place.

I was assuming that that we would only support adding layers at initialization time, but this actually allows additional layer modules to be imported dynamically at run time, which certainly seems very flexible!

This seems to take care of the javascript side. Is there a corresponding "command" or function to be added on the pydeck/python side?

modules/jupyter-widget/src/dynamic-registration.js

ibgreen · 2020-02-06T16:49:51Z

modules/jupyter-widget/src/widget.js

@@ -125,9 +126,21 @@ export class DeckGLView extends DOMWidgetView {

    this.model.on('change:json_input', this.valueChanged.bind(this), this);
    this.model.on('change:data_buffer', this.dataBufferChanged.bind(this), this);
+    this.model.on('change:custom_layers', this.addNewCustomLayers.bind(this), this);


change:custom_layers - We only support layers right now, not other types of configuration objects (e.g. constants).

I think it is fine to only support layers for now, though I wouldn't mind if we designed it with more generality/expandability in mind, e.g. change:add-configuration, which takes a JSON configuration object, and renamed the method to this.add<Custom>JSONConfiguration().

(Completely generalizing things would seem to require conventions for how to bundle a module so the various types of exports are discoverable on dynamic import, and I don't think that would need to be solved in this PR, i.e. for now it is fine to only send configuration objects with a classes field.)

ibgreen · 2020-02-06T16:53:54Z

modules/jupyter-widget/src/dynamic-registration.js

+    // Opinionated choice, requires that the user load only one layer at a time
+    // and that layer must be the sole default export of the library
+    // TODO better choice here?
+    jsonConverter.configuration.classes[className] = window[className].default;


You are updating an existing configuration. This code is a little hacky, I don't think it would be unreasonable to provide a JSONConverter.mergeConfiguration(newConfiguration) API for merging in a new configuration object.

ibgreen · 2020-02-06T16:58:42Z

modules/jupyter-widget/src/widget.js

    this.dataBufferChanged();
  }

+  addNewCustomLayers() {
+    if (this.model.get('custom_layers')) {


Can we use the same naming as JSON config objects?

this.model.get('classes`) ...

modules/jupyter-widget/src/dynamic-registration.js

Pessimistress · 2020-02-06T20:00:43Z

modules/jupyter-widget/src/dynamic-registration.js

+    // Opinionated choice, requires that the user load only one layer at a time
+    // and that layer must be the sole default export of the library
+    // TODO better choice here?
+    const classConstructor = window[className].default;


How is the user supposed to bundle their code? Add an example/test.

The test is addressed in #4267 and I'll add documentation/an example directory elsewhere

modules/jupyter-widget/src/dynamic-registration.js

Pessimistress · 2020-02-06T20:03:21Z

modules/jupyter-widget/src/create-deck.js

@@ -41,8 +42,13 @@ const jsonConverter = new deck.JSONConverter({
  configuration: jsonConverterConfiguration
 });

-export function updateDeck(inputJSON, deckgl) {
-  const results = jsonConverter.convert(inputJSON);
+export function updateClasses({className, resourceUri, onComplete}) {


Why do we need a function that directly calls another? Where is onComplete used?

onComplete is used in the test associated with this function

Pessimistress · 2020-02-11T00:39:17Z

bindings/pydeck/pydeck/widget/_frontend.py

@@ -9,4 +9,4 @@
 module_name = "@deck.gl/jupyter-widget"
 # module_version is the current version of the module of the JS portion of the widget
 # It appears to be important only for JupyterLab and ignored for Jupyter Notebooks
-module_version = "^8.0.0"
+module_version = "8.1.0-alpha.1"


Can we not hardcode any version?

Handled in a separate PR, this is only for development purposes, I'll change it back

modules/jupyter-widget/src/create-deck.js

modules/jupyter-widget/src/index.js

modules/jupyter-widget/src/widget.js

modules/jupyter-widget/webpack.config.js

test/modules/jupyter-widget/create-deck.spec.js

modules/jupyter-widget/src/widget.js

Pessimistress · 2020-02-12T02:22:50Z

modules/jupyter-widget/src/create-deck.js

+  });
+}
+
+function loadExternalClasses({libraryName, resourceUri, onComplete}, loadResource = loadScript) {


Add a comment: loadResource is for testing only

Nit: I personally try to avoid mixing positional and named parameters.

Pessimistress · 2020-02-12T02:24:49Z

modules/jupyter-widget/src/widget.js

+      const customLibraries = this.model.get('classes');
+      for (const obj of customLibraries) {
+        // obj is an object of {libraryName: <string>, url: <string>}
+        const [libraryName, resourceUri] = Object.entries(obj)[0];


obj.libraryName,
obj.url

They're arriving as a list of dictionaries from the server in something like this format:

[{"libraryName1": "url1"}, {"libraryName2": "url2"}]

Don’t you have control of the format on the server side?

I do but I'm personally partial to the more concise form of

[{"libraryName1": "url1"}, {"libraryName2": "url2"}]

instead of

[{"libraryName": "libraryName1", "resourceUri": "url1"}, {"libraryName": "libraryName2", "resourceUri": "url2"}]

It's anyone's choice, however–happy to switch it over.

Think of each item as an entry in a database. Predictable data structured makes your code a lot more readable. Using the key name as the library name is implicit, and requires additional documentation in the source. Explicitly calling out libraryName leaves no ambiguity.

If you insist on using keys as library names, which is fine, then there's no reason to have an array. All entries can be merged into one dictionary. Object.entries(obj)[0] is a bad pattern because you are relying on the existence and uniqueness of the object's content. It will easily crash if the input is malformed.

ibgreen

See inline comment, this seems to only support importing a single layer per module, (and no other types of symbols, such as enums etc).

From our use case, even with this limitation, this is a big step up. That said we will probably soon want to import custom modules with multiple layers, and perhaps also enums, constants etc.

For longer-term, we also have requests around being able to import alternative base map modules into pydeck, and doing that using a generalization of this dynamic injection mechanism could be very elegant. Of course we would need to solve a number of questions around how those are integrated (a discussion partially started in your pydeck multiview PR).

For a possible follow-up PR: long-term would it make sense to route all deck.gl layer module loading through this function, not just custom modules? That would make this code into a primary part of the library, instead of a "fragile" edge case.

ibgreen · 2020-02-12T13:44:44Z

modules/jupyter-widget/src/create-deck.js

+  });
+}
+
+function loadExternalClasses({libraryName, resourceUri, onComplete}, loadResource = loadScript) {


Nit: I personally try to avoid mixing positional and named parameters.

ibgreen · 2020-02-12T13:52:14Z

bindings/pydeck/pydeck/settings.py

+        one could load it into pydeck by doing the following:
+
+        ```
+        pydeck.settings.custom_classes = [{'TagmapLayer': 'https://demourl.libpath/bundle.js'}]


I suppose that we since we haven't yet implemented the configuration "back-channel" message back to Python (which would let Python about what JS classes are available) the user has to duplicate that information here.

It would be valuable to be able to import multiple classes per module (in fact it would be valuable to be able to import also constants etc).

Therefore I am wondering if a class-name-to-url scheme is sufficient, or if it should be

pydeck.settings.custom_classes = { 'https://demourl.libpath/bundle1.js': [ classes: ['TagmapLayer', 'Layer2'], enums: ... ], 'https://demourl.libpath/bundle2.js': [ classes: ['MyCustomLayer'], ] };

Again, ideally long-term we should have the back-channel message and not require listing the contents of the packages on the JS side.

The latest implementation of this allows for multiple classes. I'll be updating the docstring here to read {"tagmapLibrary": ...url...} instead.

ibgreen · 2020-02-12T13:54:43Z

modules/jupyter-widget/src/create-deck.js

@@ -10,19 +10,19 @@ import * as deck from './deck-bundle';

 import GL from '@luma.gl/constants';

-function extractClasses() {
+function extractClasses(library) {


Please add a more comprehensive header describing what this function does.

// Get classes for registration from standalone deck.gl

this makes it sound like this is for the top-level deck.gl module only

modules/jupyter-widget/src/widget.js

ajduberstein · 2020-02-18T18:45:17Z

Will merge after #4267 is merged

…loading (#4267)

ajduberstein changed the title ~~[WIP] Add support for dynamic deck.gl custom Layer registration~~ pydeck: Add support for dynamic deck.gl custom Layer registration Feb 6, 2020

ajduberstein requested a review from ibgreen February 6, 2020 03:27

ajduberstein changed the title ~~pydeck: Add support for dynamic deck.gl custom Layer registration~~ pydeck and @deck.gl/jupyter-widget: Add support for dynamic deck.gl custom Layer registration Feb 6, 2020

ibgreen approved these changes Feb 6, 2020

View reviewed changes

Pessimistress requested changes Feb 6, 2020

View reviewed changes

ajduberstein force-pushed the ajd/custom-layer-loader branch from 3b4dbea to 0265914 Compare February 10, 2020 23:44

Pessimistress reviewed Feb 11, 2020

View reviewed changes

modules/jupyter-widget/src/widget.js Outdated Show resolved Hide resolved

Pessimistress reviewed Feb 12, 2020

View reviewed changes

ibgreen approved these changes Feb 12, 2020

View reviewed changes

ajduberstein force-pushed the ajd/custom-layer-loader branch from 19b6855 to 7d3449b Compare February 13, 2020 00:29

ajduberstein mentioned this pull request Feb 13, 2020

pydeck and @deck.gl/jupyter-widget: Add render test for custom layer loading #4267

Merged

Pessimistress approved these changes Feb 13, 2020

View reviewed changes

modules/jupyter-widget/src/widget.js Outdated Show resolved Hide resolved

Andrew Duberstein added 15 commits February 14, 2020 13:11

WIP Add support for dynamic Layer class registration

790278b

Finish custom layer registration

a03d25b

Use promise syntax

09f0367

Change merge syntax

6b97c03

Update widget name

d5671a4

Add global pydeck settings

100085f

Use fetch instead of xhr

ed1c57e

Update for testing

4c9bc6d

Add test for merge

6d8c981

Strike only; test the negative case

667c31b

Add comment

7a8cf61

Rename function

c72c1a5

Update create-deck

e254361

Update to promise without fetch

697ea9f

Update

86f0163

Andrew Duberstein added 4 commits February 14, 2020 13:11

update docstring:

25fdb65

Add standalone html support and settings.py tests

528c147

Change format of custom libraries array

6c64d23

Update

89090a5

ajduberstein force-pushed the ajd/custom-layer-loader branch from 075ce33 to 89090a5 Compare February 14, 2020 21:11

pydeck and @deck.gl/jupyter-widget: Add render test for custom layer …

4c98bd0

…loading (#4267)

ajduberstein merged commit 9fc27f9 into master Feb 20, 2020

ajduberstein deleted the ajd/custom-layer-loader branch February 20, 2020 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pydeck and @deck.gl/jupyter-widget: Add support for dynamic deck.gl custom Layer registration #4233

pydeck and @deck.gl/jupyter-widget: Add support for dynamic deck.gl custom Layer registration #4233

ajduberstein commented Feb 4, 2020 •

edited

Loading

coveralls commented Feb 4, 2020 •

edited

Loading

ajduberstein commented Feb 6, 2020

ibgreen left a comment

ibgreen Feb 6, 2020

ibgreen Feb 6, 2020

ibgreen Feb 6, 2020

Pessimistress Feb 6, 2020

ajduberstein Feb 13, 2020

Pessimistress Feb 6, 2020

ajduberstein Feb 11, 2020

Pessimistress Feb 11, 2020

ajduberstein Feb 11, 2020

Pessimistress Feb 12, 2020

ibgreen Feb 12, 2020

Pessimistress Feb 12, 2020

ajduberstein Feb 13, 2020

Pessimistress Feb 13, 2020

ajduberstein Feb 13, 2020

Pessimistress Feb 13, 2020 •

edited

Loading

ibgreen left a comment

ibgreen Feb 12, 2020

ibgreen Feb 12, 2020

ajduberstein Feb 12, 2020 •

edited

Loading

ibgreen Feb 12, 2020

ajduberstein commented Feb 18, 2020

pydeck and @deck.gl/jupyter-widget: Add support for dynamic deck.gl custom Layer registration #4233

pydeck and @deck.gl/jupyter-widget: Add support for dynamic deck.gl custom Layer registration #4233

Conversation

ajduberstein commented Feb 4, 2020 • edited Loading

Background

Change List

coveralls commented Feb 4, 2020 • edited Loading

ajduberstein commented Feb 6, 2020

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pessimistress Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

ibgreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajduberstein Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajduberstein commented Feb 18, 2020

ajduberstein commented Feb 4, 2020 •

edited

Loading

coveralls commented Feb 4, 2020 •

edited

Loading

Pessimistress Feb 13, 2020 •

edited

Loading

ajduberstein Feb 12, 2020 •

edited

Loading