sparksql and trino LSP - Githubissues

cccs-jc commented 2 years ago

@krassowski we have developed a jupyterlab extension which provides code completion from Spark sql and Trino.

You can see the features of the extension here https://github.com/CybercentreCanada/jupyterlab-sql-editor

You'll notice that we have registered LSP support via external configurations. This works great but is burdensome. Ideally we would like these configurations to be integrated into jupyterlab-lsp.

Is that something that would be welcomed? thanks

krassowski commented 2 years ago

This looks awesome. Sure, let's think what parts we can move here. We now have an entire organization so things which would not fit into the main repo could land as a separate repository.

If you have any suggestions on reworking the APIs to make it easier to depend on please let us know too.

cccs-jc commented 2 years ago

That's great. The parts I was thinking would make sense to integrate into juypterlab-lsp are the configurations.

Specifically in this section https://github.com/CybercentreCanada/jupyterlab-sql-editor#configure-sql-language-server-startup-scripts

This file registers the language server along with their mime types ~/.jupyter/jupyter_server_config.py

The other part is configuring the sql-language-server via this override /opt/conda/share/jupyter/lab/settings/overrides.json

That's about it. I know many other language server are registered so I can follow these examples and create a PR.

The second section I'm not sure how to do?

The third section configures the ipython kernel however the default values will work out of the box so it's not necessary to include any of these configurations into jupyterlab-lsp https://github.com/CybercentreCanada/jupyterlab-sql-editor#pre-configure-magics-optional

krassowski commented 2 years ago

Just to make sure, did you know that you can just drop a file to Jupyter config directory and this will get detected? This is what https://github.com/jupyter-lsp/json-lsp / https://github.com/jupyter-lsp/yaml-lsp and robotframework do, see https://github.com/robocorp/robotframework-lsp/blob/master/robotframework-ls/src/robotframework_ls/ext/jupyter_lsp.py and https://github.com/robocorp/robotframework-lsp/blob/57b4b2b14b712c9bf90577924a920fb9b9e831c7/robotframework-ls/src/setup.py#L165-L167

krassowski commented 2 years ago

But sure if you prefer it, we would be happy to accept your language server specification files into jupyterlab-lsp. That would usually entail adding at least a minimal test to at continuously verify that it still works/connects on supported versions of Python etc (as we include all servers with specification files in our documentation as "supported"). If it requires installing a lot of dependencies it might be problematic, but if the dependencies are already installed on the common OSes we test against (Windows/Mac/Ubuntu), or relatively lightweight and already packaged on conda-forge (or if you are willing to help with packaging) then we would very likely merge thes specs.

As for overrides, I'm not sure how we can simplify that for you but I will give it a think.

bollwyvl commented 2 years ago

howdy, see there's on-going discussion i don't want to distract too much from, but wanted to throw some notes:

Looks like cool stuff. In addition to the specs note below, for the language_servers setting in the frontend, there are some challenges for sure. I tried to sum it up in this upstream:

https://github.com/jupyterlab/jupyterlab/issues/11667

Like the settings UI, if we did land this, it would be a Lab 4.0-era change... and given 4.0 will be a fairly big lift for this extension, it might be a while before we could take advantage of it.

drop a file to Jupyter config directory

Yep, we went to some trouble to make this doable, if not exactly easy: https://jupyterlab-lsp.readthedocs.io/en/latest/Extending.html#language-server-specs

land as a separate repository.

If existing sql-based stuff is conflicting, we might have to start moving more of these language-specific things into sub-applications, or putting better gates on them. This might temporarily increase the complexity of distribution and testing, but anything we can do to start brining down core build/test complexity will be good.

However, this could mean having ever-increasing specs in core, but moving the testing of that part of the full application in sub-projects with much smaller environments... we would could then have some special CI runs that test across all the sub-languages, but in parallel jobs.

cccs-jc commented 2 years ago

I like how the https://github.com/jupyter-lsp/json-lsp registers it's language spec. It looks like will execute the python code that registers the spec. I need this because sql-language-server is a node process so I need to dynamically find the npm module. I'll give that a try.

However it would be interesting for this jupyterlab-sql-editor extension to be mentioned in the jupyterlab-lsp documentation.

bollwyvl commented 2 years ago

I need this because sql-language-server is a node process so I need to dynamically find the npm module.

Look further in that example, i guess: ending up with a usefully-installable language server usable from other LSP clients is a good goal. In this case, the entry_points = {console_scripts: ...} leaves a "well-behaved" entry_point.

interesting for this jupyterlab-sql-editor extension to be mentioned in the jupyterlab-lsp documentation.

PRs welcome. Note that if the spec gets added, much of that language server part of the docs is automatically demonstrated based on the information provided in the spec. And they get tested. This helps us stay confidence that at least, when we last built it, it works. As things get harder, less reproducible, I get ... less comfortable about hyping it.

Outside of specs, we don't really have a data-driven place for downstreams, yet... there are a few narrative pieces for e.g. the metals example. It's hard, as unless we have the downstreams under test, we might be sending users to broken/unmaintained places...

bollwyvl commented 2 years ago

Welp, it looks like we can try out the lab settings side of the house now:

https://github.com/jupyterlab/jupyterlab_server/releases/tag/v2.9.0

So, concretely, you'd have data_files like:

data_files=[
  ("share/jupyter/lab/static/settings/overrides.d/", ["sparksql-and-trino-lsp.json"]),
  # ... the rest of the extension 
]

where that json file is like:

{
     "@krassowski/jupyterlab-lsp:plugin": {
        "language_servers": {
             # the stuff
        }
    }
}

cccs-jc commented 2 years ago

Ha interesting once again. So data_files is a means to install files in your python setup process. My background is in Java so Python/Typescript is a bit new to me.

We have built the installation of overrides.d in our docker image build process but now that I know I can use python setup process to do that. I will use this mechanism. This way people installing the our extension will benefit from a default configuration out of the box.

BTW: I've followed the entry_points technique and it works quite well. I was able to find the sql-language-server node module and start it. It's all python so it's very flexible. I added 2 entry_points and 2 console_scripts one for each sparksql and Trino.

cccs-jc commented 2 years ago

I have tried to add to the overrides.d, however this looks to be a new feature not release yet. Correct? https://github.com/jupyterlab/jupyterlab_server/issues/223

What alternative do I have while this new feature becomes more mainstream? Dropping an overrides.json file ?

cccs-jc commented 2 years ago

According to this code the place to drop files in overrides.d would actually be

share/jupyter/lab/settings/overrides.d

correct?

I did that but still it's not being picked by JupyterLab. Any ideas how to track this down..

krassowski commented 2 years ago

Investigating jupyter --paths, and adding some log statements in a local copy of https://github.com/jupyterlab/jupyterlab_server/blob/main/jupyterlab_server/settings_utils.py#L270?

For reference here is a live example of overrides.json at work: https://github.com/jupyterlab/jupyterlab-plugin-playground/pull/26/files

bollwyvl commented 2 years ago

Sorry, missed this at some point due to holiday stuff.

alternative do I have while this new feature becomes more mainstream

There are now several release out that have the feature included (the above link is to the first release). If you more-or-less know all the users and or control the environment, your package can depend on that version, you can now specify a pin on jupyterlab_server >=2.10.2, and you can ship your file.

If you are uncomfortable with that, your sensible options are:

thorough documentation (as you've done)
- this is pretty complicated, though that kind of seems par for language server configuration, even in vscode :shrug:
offer a custom jupyterlab plugin which updates the user's preferences
- this plugin could store whether it has been configured in its own settings
- if it is found to not be configured, you can pop a dialog asking whether they'd like it configured, or to not be asked again
- jupyterlab command, e.g. Configure Sparksql Language Server
- and have it pop that once
offer a custom CLI command that would update the settings

What you definitely should not do is ship an overrides.json as part of your package... pip, conda, etc. don't handle conflicts in config files basically at all, hence why the overrides.d feature was added in the first place. That file is intended to be owned used by the user/deployer.

cccs-jc commented 2 years ago

I've got it to work with the proposed solution of data_files https://github.com/CybercentreCanada/jupyterlab-sql-editor/blob/1dab518641a79e07ec8da4abaf491c7f6621b619/setup.py#L28

I will soon publish the package to Pypi and then get in touch to how best make a mention of this LSP extension in your documentation. Sounds good?

cccs-jc commented 2 years ago

The package is on pypi https://pypi.org/project/jupyterlab-sql-editor/

@krassowski on what page could we reference this LSP server within jupyterlab-lsp

thanks

jupyter-lsp / jupyterlab-lsp

sparksql and trino LSP #716