💭 streamline server starting, binder, architectural musings

[bollwyvl]

Really great work on this repo! 🌠

Initially inspired by #2, this is a bit of a high-velocity wander through your code, and not really intended to be merged, but the binder works (though just for python)!

nb: this was updated to point at urlpath, and launches lab, but sometimes still won't open the example

My 🥇 is to be able to teach people to develop full hub-, server- and labextensions directly in Lab (running on a hub), and this repo is the closest i've seen to achieving that polyglot goal. So a quick run-down on things to get over some of the initial bars:

• uses jupyter-server-proxy to launch the LSP multiplexer (when first requested)
  • this configuration could be made pip installable for a no-config setup, as j-s-p supports entry_points
  • i think we'd eventually want to shoot for a python version of that feature that could be PRd to notebook
  • either way, probably would want a standalone (conda) package for it rather than relying on nodejs-of-circumstance
  • i was going to load up a few more language servers (yaml, json, typescript) but didn't quite get around to it, see below re: node pain
• upgrades to lab 1.1.1 (though 1.2.0 is apparently just around the corner)
  • upgrade typescript, prettier, etc.
• adds a conda opinion to binder
  • yeah, you can depend on nodejs being in binder, but I like to keep those particular ducks in a row
• makes use of jlpm over npm where possible
  • checks in a yarn.lock (again, see above)
• adds something to schema (chokes otherwise in 1.1.1)

I'd really love to see your work PRd to jupyterlab/jupyterlab core, and usable by non-python-lsp-typescript ninjas.

To that end, here's some fairly concrete (but opinionated) architectural ideas:

• break jupyterlab-lsp into multiple packages, use lerna and manage with typescript project references
  • see lab core for examples, e.g. you'd have a non-distributed meta and everything would be built as a side effect of building that, so it wouldn't be any slower (just a bit more boilerplate)
• merge this repo with jupyterlab_go_to_definition
  • i wanted to make typescript work, and found that some of the language detection stuff lived down in CodeJumper, but didn't want to manage a hot submodule, etc.
  • though it might already work if i mount application/typescript in servers.yml, no telling
• reduce complexity of index.ts, perhaps with an ILSPManager and LSPManager implementation that can act as a touch point
  • make the manager (and extensions) lazy load more things with await import... every 100kb counts, and this+dependencies clocks in at a couple of this
• firewall all the IPython-specific stuff into a separate sub-package that uses a ILSPManager api to register crazy stuff
  • or better still, figure out a declarative JSON syntax that can be served from a serverextension, such that a kernelspec or kernel_info could carry the info
• config (but you've already noted that)

Keep up the good work, and thanks again!

[krassowski]

Thank you, this is fantastic work!

I agree with your suggestions - many things are new ideas, some already thought through - anyway this is great feedback and thank you for taking the time to go through the code and write it down. Some quick points on architectural ideas:

• break jupyterlab-lsp into multiple packages, use lerna and manage with typescript project references

• It might be a good idea. I have no way of telling (limited experience in TS realm) but I would probably postpone this until some time later after most of the exploratory work is finished (I keep findings things which need to be refactored because I did not imagine that those will be needed - and finding more of those - with help of the users - is the priority right now, in order to stablise the interfaces once a satisfactory state is achieved).

• merge this repo with jupyterlab_go_to_definition

• Thanks for reminding me about the language deduction code - I though I broke the dependency from the jumper for that, must have been some work which was not committed. Anyway, I will work on that and iit should not have any major impact in the future.

• I was thinking about splitting the important stuff from the jumping extension (like the actual jump operation implementation) into another extension which could be migrated to @jupyterlab org. It could be named jupyterlab-jumplib. This would mean that the language-specific constructs and the maintenance burden of those would still live outside of the core org (and that the lsp would depend on a trusted extension).

• Another idea would be to merge jupyterlab-jumplib into jupyterlab-lsp and provide the language-specific definitions as part of jupyterlab-lsp-python, jupyterlab-lsp-r etc. This would, however, mean that the jupyterlab_go_to_definition should be archived once jupyterlab-lspreaches stability.

• reduce complexity of index.ts, perhaps with an ILSPManager and LSPManager implementation that can act as a touch point

• Agreed 100%.

• firewall all the IPython-specific stuff into a separate sub-package that uses a ILSPManager api to register crazy stuff

• Agreed! As noted in technical notes of Magics_and_rpy2.ipynb:

In the future the "included batteries" may be moved out to separate extensions i.e. jupyterlab-lsp-ipython for default IPython magics support and jupyterlab-lsp-rpy2 for rpy2 support.

• or better still, figure out a declarative JSON syntax that can be served from a serverextension, such that a kernelspec or kernel_info could carry the info

I would lean towards the previous idea, as the custom extensions would be able to provide classes implementing IForeignCodeExtractor interface with custom methods, rather than being restricted to regular expressions.

--

I am happy to merge this PR as-it-is after releasing 0.5.0, as having a binder example up and running could be beneficial to potential users - they could quickly run it and assess whether it works for them and give feedback without the need to install the extension manually.

PS. The binder badge does not work for me right now (404), but I see from the code that it should be working :)

[krassowski]

Just highlighting some code/documentation fragments relevant to the discussion on lsp language-specific features:

Extraction of foreign code:

https://github.com/krassowski/jupyterlab-lsp/blob/7a0e107f126ba866f10cfa97bec3004dbf63aeb3/src/extractors/types.ts#L18-L59
Current extractors defaults: link.

Magics:

Defaults and docs:
https://github.com/krassowski/jupyterlab-lsp/blob/7a0e107f126ba866f10cfa97bec3004dbf63aeb3/src/magics/defaults.ts#L3-L35

Also, about the magics a fragment from krassowski#3 (comment):

• [alternatively] we could ask linters to respect all the fancy features introduced by interactive kernels, but there will always be a gap between the introduction of new magic to a specific kernel and all the possible linters picking up on that:
  • for example, LSP for Python (pyls) depends on over a dozen different specialized linters, many of which may consider the ipython feature requests to be too specific for their scope.
  • the LSP for R has much, much slower rate of development, possibly because most of the casual R users, while excelling in statistical programming are not programmers familiar with advanced general programming aspects.
• there are some transpiling solutions (IPython → plain python file), which we could use as a reference, to avoid re-inventing a wheel

another argument for the custom handling of magic syntax is that the users can define their own magics which have different side effects and those are unlikely to be ever added to the server-side linters.

Language-specific jumping

No code in LSP at the moment, however:

• it might be beneficial to retain the functionality of client-side, language-specific jumping as implemented in the jupyterlab_go_to_definition extension as it works even when the LSP server is disconnected.
• there is the question of kernel-side language-specific jump-target detection (also implemented in jupyterlab_go_to_definition - for python) - for obvious reasons it can achieve much more than will be ever possible for the LSP server (e.g. because the order of cell execution in Jupyter notebooks is non-linear).

Thus the proposition to offer jupyterlab-lsp-python, -r, etc. extensions which will contain such functionalities.

[bollwyvl]

Thanks for the speedy review! Glad these hacks could be of interest. I'll make sure to fix the binder badge. I was really just trying to Make It Work with some other stuff that needed lab 1.1, so there are some unnecessary diffs (formatting, etc). I can break up the pr into smaller, more coherent pieces: to answer the mail on #2, just the j-s-p setup, as a python package, with a default lab config value (one should be able to do their own thing, no?) and get binder working. It can continue to detect the proxy in staging, but I'd probably also go do a conda-forge package, so there was at least _a_ way to install the whole thing simply. Regarding linting of kernel-specific syntax: indeed, on my previous foray into implementing part of lsp (@deathbeds/lintotype) I forewent running an actual lsp server, and had the kernel itself provide the lsp data model over a custom comm, as then the syntax awareness need only be written in one place. This was simpler from a notebook user perspective, as one didn't need another server running, and could reconfigure their preferences interactively in their language of choice, but useless for large code bases (e.g jupyterlab typescript) for which one really only wants static analysis. I think a hybrid that was able to delegate some things (at least configuration, but maybe behavior) to a kernel where appropriate, but generally use lsp, would be an interesting approach. That's what the JEP process is for, I guess: we _can_ do anything, sure, but doing the _right_ thing in the most sustainable way is the goal!

…

On Fri, Sep 6, 2019, 07:41 M. Krassowski ***@***.***> wrote: Just highlighting some code/documentation fragments relevant to the discussion on lsp language-specific features: https://github.com/krassowski/jupyterlab-lsp/blob/7a0e107f126ba866f10cfa97bec3004dbf63aeb3/src/magics/defaults.ts#L3-L35 https://github.com/krassowski/jupyterlab-lsp/blob/7a0e107f126ba866f10cfa97bec3004dbf63aeb3/src/extractors/types.ts#L18-L59 Also, about the magics a fragment from #3 (comment) <krassowski#3 (comment)> : - [alternatively] we could ask linters to respect all the fancy features introduced by interactive kernels, but there will always be a gap between the introduction of new magic to a specific kernel and all the possible linters picking up on that: - for example, LSP for Python (pyls) depends on over a dozen different specialized linters, many of which may consider the ipython feature requests to be too specific for their scope. - the LSP for R has much, much slower rate of development, possibly because most of the casual R users, while excelling in statistical programming are not programmers familiar with advanced general programming aspects. - there are some transpiling solutions (IPython → plain python file), which we could use as a reference, to avoid re-inventing a wheel another argument for the custom handling of magic syntax is that the users can define their own magics which have different side effects and those are unlikely to be ever added to the server-side linters. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/krassowski/jupyterlab-lsp/pull/22?email_source=notifications&email_token=AAALCRAY4ZBQ2GE4B2GPTTLQII6WXA5CNFSM4IUEHXL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6CSZVQ#issuecomment-528821462>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAALCRFV4AIWULDXZWSEIMTQII6WXANCNFSM4IUEHXLQ> .

[krassowski]

Wow, there are some great features at https://github.com/deathbeds/lintotype - sadly I wasn't aware of your work before. I am captivated by the class diagram screenshot!

As we stray into the higher-level design discussion, I would note that the approach of directly connecting to the LSP server, rather than using a kernel relay, has a benefit of allowing for easy and direct @sourcegraph integration (which was brought up a couple of times in the LSP-related discussions; I am indifferent to their service and business model - it has pros & cons).

Otherwise, I see your approach of maintaining the notebook document model inside of the kernel as equally good and agree that a hybrid approach may be the best way out:

• in the short term, we would not need a custom kernel extension to bring an LSP to the users
• potential for sourcegraph integration without an unnecessary delay (I was working with JL running on another continent for two years and don't want to deny the pleasure of seamless remote work to others)
• in the long term, the kernel-specific extensions could provide more advanced features, possibly things as amazing as the class diagram that you showcased.
• and of course, for the FileEditor we want to keep things simple because if we introduce another kernel for each open file, it might have a negative performance penalty (I already feel the performance hit due to the pyls or lintr LSP servers alone - IMO one of them has a memory leak).

Also, please note that the language-specific extensions I proposed above were intended to be client-side extensions (like in the JupyterLabFrontend typescript); we indeed discussed the kernel-side extension in #2, but I will reiterate that for some features it might make sense to have the language-specific code in the client extension (as long as the notebook model lives in the client, of course).

EDIT: to clarify - I still support the idea of having kernel-side extensions for jupyterlab-lsp as discussed in #2 (and as demonstrated with jupyter-server-proxy in this PR). Just wanted to highlight the distinction to the future readers (including future me).

PS. don't worry about formatting - a run of a prettier on the entire codebase was overdue. I need to configure some pre-commit hooks for that...
PS2. sorry if I repeat myself, just want to make sure that anyone reading this discussion, later on, is able to understand things easily despite a portion of new terminology.

[blink1073]

Great conversation and work @bollwyvl and @krassowski! I like the idea of having juptyerlab-lsp and jupyterlab-lsp-python in core, and for a language-specific extension to be able to provide goto-definition even when there is no kernel.

[bollwyvl]

Nothing to report yet on cleaning this up: did anyone have a chance to check out if the binder link actually works for them? (WFM 😬)

I did spend some time ruminating on what making a jupyter-adjacent wrapper for jsonrpc-ws-proxy to run under jupyter-server-proxy could look like:

https://gist.github.com/bollwyvl/d1885ed2a1376d32b71acee4506f7240

The idea would be that, much like we'd have some frontend-specific stuff that was labextension installed per language, some pip (or conda or whatever) installs would allow for capturing the specifics of the server location, just by adding a jupyter_config.d/my-lang.json file... or another entry_point, i suppose, though conf.d is a bit more humane.

That being said, I threw some "best effort" path detection as I understood how to add it for some web-like things, but sometimes finding node (much less a specific node_module/**/*.js is pretty much a fool's errand unless you ship it yourself, a la vscode/atom.

Onward!

[krassowski]

Nothing to report yet on cleaning this up: did anyone have a chance to check out if the binder link actually works for them? (WFM grimacing)

Works for me too now - must have been a temporary binder issue. Solving conflicts and merging!

[krassowski]

Just one note and one question. Sorry if I rushed with the merge - please feel free to send a follow-up PR if you wish to change anything.

[krassowski]

black and isort are not use at this time, but will come in handy soon (this is the plan ;)) so we can keep those here.

[krassowski]

Does this file need to be in root and in binder?

[bollwyvl]

Ah, wasn't expecting the merge. Onward! We can/should get rid of the binder directory, and just keep all that stuff in root. The jupyter_notebook_config should go away in favor of a pip installable configuration of jupyter-server-proxy, as suggested above. I actually spent some more time on that... Basically breaking up the autodetect things into separate entry_points, while still allowing config to win. Black, isort, and mypy, and their pyls wrappers, were just to see what came over the wire if they were installed. Mypy complains a lot, and presumably we're not yet handling the code modification messages from the others.

…

On Sun, Sep 15, 2019, 10:41 M. Krassowski ***@***.***> wrote: ***@***.**** commented on this pull request. Just one note and one question. Sorry if I rushed with the merge - please feel free to send a follow-up PR if you wish to change anything. ------------------------------ In binder/environment.yml <https://github.com/krassowski/jupyterlab-lsp/pull/22#discussion_r324466083> : > @@ -0,0 +1,20 @@ +name: jupyterlab-lsp + +channels: + - conda-forge + - defaults + +dependencies: + - black black and isort are not use at this time, but will come in handy soon (this is the plan ;)) so we can keep those here. ------------------------------ In jupyter_notebook_config.py <https://github.com/krassowski/jupyterlab-lsp/pull/22#discussion_r324466163> : > @@ -0,0 +1,15 @@ +from pathlib import Path Does this file need to be in root and in binder? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/krassowski/jupyterlab-lsp/pull/22?email_source=notifications&email_token=AAALCRDQU2TI4ID42JE5MWDQJZCQJA5CNFSM4IUEHXL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEYB5JQ#pullrequestreview-288366246>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAALCRE3TZEVCEL3R6A3IHTQJZCQJANCNFSM4IUEHXLQ> .

[krassowski]

Sounds good! Feel free to continue your awesome work and contribute it here if it suits you :) I will be mostly working on breaking down the LSP features into smaller, testable submodules, upstream PRs and config system in the near future. This should keep the potential merge conflicts to a minimum.

Mypy works for me great - if there are typings available - and otherwise complains, indeed. The others are not handled yet but will be in the next release.