Open tgodzik opened 2 years ago
@tgodzik This is something that I'd love to have in metals! Depending on how hard it is, I would consider a tilt at it :-)...
I've done some experiments in the past, which prove that one can indeed run scala in a VSCode notebook, simply by wheeling in the Almond kernel. However, it's a "raw" experience right now.
I hope, that could be improved in a couple of ways, which I believe to be 'independent'.
convince the notebooks to recognise the metals LSP server. I did manage to get metals working in a jupyterhub via LSP, so something is at least close already. My (naive) hope is that this might just be a case of trawling some documentation to get a config right. This would be cool in its own right, even if I didn't mange to go further.
FTW, what would be amazing, would be if it was possible to execute a notebook cell, in the context of a project - same like kicking up an ammonite / scala console out the build tool. I fear, that this may be a rather difficult problem. My hope would be that it was possible to 'trick' almond into having the project context, without having to change almond itself. I am not yet sure, how... but hope springs eternal.
Is this something you'd devoted any thought to?
If so, is the above rational? On a scale of 1-10, how hard would you perceive these problems to be? Something much above 5, and it's likely I'm out of my skill zone, sadly.
If they aren't too bad, would you be willing to guide an effort along these lines, and / or set out a solution sketch? I'm not familiar with the project, so if you had a hint on where to look, it would be great, or any other thoughts?
@Quafadas thanks for showing the interest! This is actually part of GSoC projects this year, but there it's possible it will not be picked. I will let you know once we have the results.
convince the notebooks to recognise the metals LSP server. I did manage to get metals working in a jupyterhub via LSP, so something is at least close already. My (naive) hope is that this might just be a case of trawling some documentation to get a config right. This would be cool in its own right, even if I didn't mange to go further.
It already recognises the notebook cells written in Scala from what I've seen, but there is no support for the particular URI, which we would need to work on (we get the exceptions as shown in the description)
FTW, what would be amazing, would be if it was possible to execute a notebook cell, in the context of a project - same like kicking up an ammonite / scala console out the build tool. I fear, that this may be a rather difficult problem. My hope would be that it was possible to 'trick' almond into having the project context, without having to change almond itself. I am not yet sure, how... but hope springs eternal.
I might be possible to execute almond with the proper classpath and settings to make it work, but I am not familiar with the almond API right now.
If so, is the above rational? On a scale of 1-10, how hard would you perceive these problems to be? Something much above 5, and it's likely I'm out of my skill zone, sadly.
I think it's around 5, but mostly it would be getting all the existing tools to work together, so a bit of things to figure out and change, but nothing that would require large scale rewrites.
If they aren't too bad, would you be willing to guide an effort along these lines, and / or set out a solution sketch? I'm not familiar with the project, so if you had a hint on where to look, it would be great, or any other thoughts?
I will let you know if this project gets to be a part of gsoc, if not we can try and figure it out together :smile:
This is actually part of GSoC projects this year
Ohhhh... excellent.
Thanks for taking the time to write back. I'll keep my fingers crossed that it gets picked up by that project!
This is actually part of GSoC projects this year
Ohhhh... excellent.
Thanks for taking the time to write back. I'll keep my fingers crossed that it gets picked up by that project!
This project hasn't been picked for GSoC, so if you are interested in working on it that would be awesome! I can try and provide some pointers, but generally I think the work you already did could be automated almond setup plus making sure that the notebook file provider works (there might be already an implementation for Java, but not sure.)
This project hasn't been picked for GSoC
Oh no!
I’m away right now, so will write back in a week or so. I’d like to have a think before committing to spending a lot of time, and probably do need to figure out just how long it might take. I do think this would be a very cool thing to have though!
On Mon, 23 May 2022 at 10:06, Tomasz Godzik @.***> wrote:
This is actually part of GSoC projects this year
Ohhhh... excellent.
Thanks for taking the time to write back. I'll keep my fingers crossed that it gets picked up by that project!
This project hasn't been picked for GSoC, so if you are interested in working on it that would be awesome! I can try and provide some pointers, but generally I think the work you already did could be automated almond setup plus making sure that the notebook file provider works (there might be already an implementation for Java, but not sure.)
— Reply to this email directly, view it on GitHub https://github.com/scalameta/metals-feature-requests/issues/236#issuecomment-1134324947, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF57BUDQS3VTDED6VPKREP3VLM36XANCNFSM5GLK7ARQ . You are receiving this because you were mentioned.Message ID: @.***>
This project hasn't been picked for GSoC Oh no! I’m away right now, so will write back in a week or so. I’d like to have a think before committing to spending a lot of time, and probably do need to figure out just how long it might take. I do think this would be a very cool thing to have though! …
I think this one can be done iteratively, and I can fill in any missing pieces if it turns out you don't have time. Also, it's totally fine if you at some point decide that you can't work on it further.
@tgodzik So I'm definitely up for this, although I have no experience with this sort of tooling. Should be fun.
Sadly however for "reasons", I need to ban myself before mid-July - I have something else to focus on that I can't drop.
I will spend some time now, trying to get started with changing metals and getting over the "contributor getting started" hump.
Hopefully, I'll be posting up here around mid-July / early August, when I think I can allocate some time to this (if it's still open - which weirdly, I hope it is!).
@tgodzik I hope to have some time over the coming weeks to look at this... my plan over the coming days is to look at getting started with changing metals and then go from there...
Sure, I also mentioned it as a possible student project at EPFL, but that would be in septemeber and we can remove it from the list if you manage to take a look at it. Similar to GSoC this might also not be picked up at all.
Some pointers:
@tgodzik So, I have invested a little time and sat down with this, and have (already!) reached the stage at which I'm not sure how to proceed. This is complex for me!
For the sake of transparency, here is my branch. Changes thus far are basically trivial - add an almond version, and follow the pattern set out in the download methods. To be clear, it doesn't work!
[info] 2022.07.10 16:38:20 INFO Downloading almond (for notebook support)
[error] Exception in thread "main" coursierapi.error.SimpleResolutionError$1: Error downloading sh.almond:scala-kernel_2.13.8:0.13.0
[error] not found: /Users/simon/.ivy2/local/sh.almond/scala-kernel_2.13.8/0.13.0/ivys/ivy.xml
Because that artefact, doesn't exist. Branch https://github.com/Quafadas/metals/tree/jupyter_notebooks
Maven https://mvnrepository.com/artifact/sh.almond/scala-kernel
Given that maintenance of the almond project itself is somewhat beyond the scope of this issue... that has some implications. Here are the things I'm now unsure about.
What (I think) about the above... I hope I'm not heading in the wrong direction
If the idea was instead to use metals to construct an almond launcher, which would then be launched as an independent service, then I guess this download step would be done rather differently by using coursier to bootstrap a launcher? I am assuming, that an "independent launcher" strategy, means that it would ultimately be very difficult to get the class paths of our build into the notebook...and hence that it is not the right direction of travel?
I'm sorry this post is so long...
Which artefact to even download (!)... As almond interacts with the scala kernel, I believe it it publishes against the specific kernel versions rather than the binary versions. The versions of scala Almond supports / publish will not intersect with metals published / supported versions except through chance.
That's a separate issue altogether, I can try ask about it. We could just have the latest supported version specified similar to how we deal currently with Ammonite. We can show a warning if a version is not yet supported
What the purpose of the download step is? Is it exactly to allow projects like almond to exist inside metals, without the explicit, and hence transitive dependance?
We don't want to always download all possible dependecies especially if we would need to download it for each Scala version, which would mean downloading every Scala version for the user even if they don't need it. The download step is used to avoid that.
I believe the artefact of interest to be https://mvnrepository.com/artifact/sh.almond/scala-kernel against the assumptions that 1.1 our aim is to instantiate one of these; https://github.com/almond-sh/almond/blob/main/modules/scala/scala-kernel/src/main/scala/almond/ScalaKernel.scala. 1.1 and hence have metals own an almond kernel... for supported versions?
We could use ShellRunner for running the specific kernel. When is the kernel actually needed? Only when running?
For mismatched almond / metals scala (x.x.x) version problem, I assume I could somehow use the coursier API to check the set of published versions of that scala-kernal artefact, and intersect this with the set of supported metals versions... and only attempt to download the intersection?
We can put that information in the build.sbt and show the users proper warnings.
This is potentially a very useful note
https://github.com/microsoft/vscode-jupyter/discussions/10813
https://jupyter-client.readthedocs.io/en/latest/kernels.html#kernel-specs
https://github.com/microsoft/vscode-jupyter/discussions/10151
In summary; this now all looks tractable, if I can figure out how to bootstrap almond out of the coursier API.
The first link says VSCode respects the kernel specs. The second link sets out the kernel specs, and suggests that a JSON file which allows one to, for example set environment variables for the notebook process.
The third link sets out how one may influence the class path through setting environment variables.
I made a slight change (pushed to branch above) to the shell runner to allow it to accept extra repositories when downloading dependancies, because almond has a dependancy on JVM repr which appears only to be available on jitpack. Easily solved.
In the metals test project, if I fire up a console and run this script, it downloads the key almond kernel dependancy. The last line even installs the kernel in (apparently)... the right place.
import scala.meta.internal.metals._
import scala.meta.internal.builds.ShellRunner
import coursierapi._
implicit val ec: scala.concurrent.ExecutionContext = scala.concurrent.ExecutionContext.global
val absPath = scala.meta.io.AbsolutePath("/Users/simon/Code/metals")
val aClient = new tests.TestingClient(absPath, scala.meta.internal.metals.Buffers())
val uConf = scala.meta.internal.metals.UserConfiguration()
val cConf = new ClientConfiguration(new MetalsServerConfig())
val t = Time.system.currentMillis()
val s = new StatusBar(() => aClient, Time.system, ProgressTicks.braille, cConf)
val sr = new ShellRunner(aClient, () => uConf, Time.system, s)
val playdoh = scala.meta.io.AbsolutePath("/Users/simon/Code/playdoh")
val almondDep = Dependency.of(
"sh.almond",
s"scala-kernel_2.13.7",
"0.13.0"
)
val jvmReprRepo = coursierapi.MavenRepository
.of("https://maven.imagej.net/content/repositories/public/")
sr.runJava(almondDep, "almond.ScalaKernel", playdoh, List("--install", "--command", "java -jar /Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/sh/almond/scala-kernel_2.13.7/0.13.0/scala-kernel_2.13.7-0.13.0.jar", "--id", "metalsAlmond", "--display-name", "metalsAlmond", "--global", "false", "--jupyter-path", "/Users/simon/Library/Jupyter/kernels/" ), false, extraRepos=Array(jvmReprRepo))
// "--copy-launcher", "true" too see what it's trying to run.
This gets tantalisingly close. It places the kernel in the right place, and it's even found by VSCode! There are two problems;
I manually figured out the path to the almond kernel artefact. Is there an obvious way to do that programmatically?
Bigger issues, is that the kernel cannot be started outside of that metals context. A manual attempt at doing so
java -jar /Users/simon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/sh/almond/scala-kernel_2.13.7/0.13.0/scala-kernel_2.13.7-0.13.0.jar
Error: Unable to initialize main class almond.ScalaKernel
Caused by: java.lang.NoClassDefFoundError: scala/Product
So what I think is happening, is that normally, coursier is bundling together all he stuff the JAR needs to launch vanilla almond. That's not gonna work for us. Based on my google-fu, https://stackoverflow.com/questions/18413014/run-a-jar-file-from-the-command-line-and-specify-classpath
I guess that I now need to tamper with class paths...
I do not know much about class paths. I don't even know if that's embarrassing or not :-(. Can I get the class path of the project metals is working on before using the shell runner? Any ideas?
- I manually figured out the path to the almond kernel artefact. Is there an obvious way to do that programmatically?
Actually, fetch
which is done in runJava
will return the full classpath, which it used to run and you can also get the scala-kernel from there.
- Bigger issues, is that the kernel cannot be started outside of that metals context. A manual attempt at doing so
Och, that is interesting, so --command
requires to set the classpath again? Is it possible to just use the same classpath as ScalaKernel was run with? If there is no other way you would need to manually get the classpath from the fetch command and give to that command, so a custom ShellRunner method would most likely be needed.
Actually, fetch which is done in runJava will return the full classpath, which it used to run and you can also get the scala-kernel from there.
Ooooo, okay, let me try to figure that out.
Och, that is interesting, so --command requires to set the classpath again?
Yes - I believe because it's ultimately VSCode which is going to launch the kernel, not us!
I'm thinking that we should use metals to "install" the right version of the kernel. The massive + point about "installing", is that it puts it "in the right place", and VSCode finds it and we're inside that infrastructure for free... basically we take ruthless advantage of all the work already done in Almond on that front :-D!
The rather negative point, is exactly that almond requires you to give it the command to start the kernel, when you install. I can imagine two solutions.
Solution 1
Figure out the minimal set of classes to launch the kernel per scala version, and feed it those.
Solution 2
We're in metals, right? So if can we get the class path of the project that the user is working on (during import build step perhaps?), and install the kernel at that moment? If so then I think it's a slam dunk.
We
But at that point, the kernel has the project context - so it ought to able to use, whatever is on that class path? Which is kind of the big goal!
Here's an example of the important kernel.json file. If no other classes are bundled, the launcher itself is like 200Kb, so I'm relaxed about having a few of them! I believe the Jupiter spec will replace the "connection file" variable with whatever VSCode sends it.
{
"argv": [
"java",
"-jar",
"/Users/simon/Library/Jupyter/kernels/scala/launcher.jar",
"--display-name",
"scala 3.0.2",
"--connection-file",
"{connection_file}"
],
"display_name": "scala 3.0.2",
"language": "scala"
}
Rather a long message with some big leaps. Did it make sense? Still thinking.
Rather a long message with some big leaps. Did it make sense? Still thinking.
That makes sense!
Give almond that class path... Copy the JAR during installation to the directory, so it knows how to launch itself, and launch via -cp launcher.jar:THE_REST And we'll have a kernel per project... with the last problem being naming... as we'll have a few of them!
Actually, maybe what the launcher needs is the classpath of the project? Or classpath + kernel jar? But anyway we should be ok installing one kernel per project (lazily if possible) and using the project name (or target ID), which shouldn't be a big issue.
Or classpath + kernel jar?
I think it's this... or at least that'll be my next attempt!
I'll aim in this direction. I'm probably not going to look at this again for a few days, but would you be able to point me at
a place in metals, where I can get at the project class path? a place in metals, where I can obtain a project name and / or target ID?
You can find an example of how we do it in https://github.com/scalameta/metals/blob/main/metals/src/main/scala/scala/meta/internal/metals/JavaInteractiveSemanticdb.scala#L55
@tgodzik Hi ... thankyou again for taking the time to write back and help... that looks perfect. still a bit delayed... and trying to find time a block of time to look at this again because I'm pretty excited about it...
if I get an actual proof of concept working, I guess that I will struggle to get it to the stage where it works properly inside metals... but first things first... baby steps!
If you have anything ready, you can send me a diff and I can offer some suggestions!
@tgodzik I haven't managed to spend enough time with this, but here is an initial (horrible) take on this. Which currently, doesn't work.
https://github.com/scalameta/metals/compare/main...Quafadas:metals:jupyter_notebooks
https://github.com/scalameta/metals/compare/main...Quafadas:metals:jupyter_notebooks
The idea is to add a command to metals "setup notebook kernel for this project". Which I hope would do exactly what is says on the tin.
I think that this is along the right lines, but has the following problems, in order of "I don't know how to solve this".
Currently, it's all run synchronously. I believe that should be tidied up and put in a future context, but you can see, that my progress is not rapid. Unfortunately.
Currently, it's all run synchronously. I believe that should be tidied up and put in a future context, but you can see, that my progress is not rapid. Unfortunately.
No worries, it's great to hear that you managed to work on it! You could create a PR inside your repository and maybe this way we could comment and discuss specifics of your changes?
I've created a PR... I hope to the right place... both marked WIP
I wrote some words about how can we add language support for notebooks
textDocument/*
textDocument/*
According to notebookCellTextDocumentFilter of LSP 3.17.
We can enable vscode to fire textDocument/*
for all jupyter-notebook, where the Cell's language is Scala.
diff --git a/src/extension.ts b/src/extension.ts
index 13b49f6f..32a99c36 100644
--- a/src/extension.ts
+++ b/src/extension.ts
@@ -291,6 +291,10 @@ function launchMetals(
{ scheme: "file", language: "java" },
{ scheme: "jar", language: "scala" },
{ scheme: "jar", language: "java" },
+ {
+ notebook: { scheme: "file", notebookType: "jupyter-notebook" },
+ language: "scala",
+ },
],
synchronize: {
configurationSection: "metals",
Like this:
[Trace - 01:09:28 PM] Received request 'textDocument/completion - (140)'
Params: {
"context": {
"triggerKind": 2,
"triggerCharacter": "."
},
"textDocument": {
"uri": "vscode-notebook-cell:/Users/tanishiking/src/github.com/tanishiking/scala3-playground/src/main/scala/Untitled-1.ipynb#W2sZmlsZQ%3D%3D"
},
"position": {
"line": 0,
"character": 2
}
}
[Trace - 00:04:24 AM] Received request 'textDocument/hover - (107)'
Params: {
"textDocument": {
"uri": "vscode-notebook-cell:/Users/tanishiking/src/github.com/tanishiking/scala3-playground/src/main/scala/Untitled-1.ipynb#W1sZmlsZQ%3D%3D"
},
"position": {
"line": 0,
"character": 8
},
"range": {
"start": {
"line": 0,
"character": 8
},
"end": {
"line": 0,
"character": 8
}
}
}
We can find the
uri
is like "vscode-notebook-cell:/Users/tanishiking/src/github.com/tanishiking/scala3-playground/src/main/scala/Untitled-1.ipynb#W1sZmlsZQ%3D%3D"
that point to a specific cell in the notebook Untitled-1.ipynb
id
of the cell (we can find it from *.ipynb
file)position
is the position inside the cellThe problems here are
For combining the cells, LSP 3.17 provide a notebookDocumentSync
feature (but I don't think it works for Metals).
The following change in scalameta/metals
enables client to fire notebookDocument/did*
(like didOpen
, didChange
and didClose
)
Those notifications send the content of the notebooks, and enable LSP servers track the content of notebooks
However, it doesn't work for Metals for 2 reasons
We should do the same thing as ScalaCli and Ammonite Script (though notebook would be even tricker)
sourceMapper.pcMapping
actualSources
using the given uri
actualSources
?
sourceAdjustment for (SemanticDB) indexing should work if we properly registered the MappedSource
for ipynb
@tanishiking I'm not familiar with how notebook cells work in LSP.
Will all LSP messages need to have their incoming URIs translated into a local filesystem version (or some more extensive mapping info)?
e.g. textdocument/didOpen
didClose
didSave
hover
codeLens
documentSymbol
etc.
And then will these local filesystem URIs have to be translated back into a notebook cell URI when the replies are sent?
@Arthurm1
Will all LSP messages need to have their incoming URIs translated into a local filesystem version (or some more extensive mapping info)?
Yes, I think all the LSP requests that requires semantic information from compiler / SemanticDB requires translation from incoming URI -> local filesystem version.
For example, consider we have the following two cells in a notebook.
// cell1
def add(a: Int, b: Int): Int = a + b
// cell2
add(1, 2)
cell2 itself isn't valid (both syntactically and semantically invalid). So, Almond / Ammonite translate the whole notebook into a combined compilable Scala source file (that is local filesystem version) (I guess). I think all the compiler related analysis should be done on that combined compilable Scala source file.
However, I don't think we need to support all the LSP requests / notifications for notebooks (for example, foldRange
might not be helpful in notebook environment) :)
And then will these local filesystem URIs have to be translated back into a notebook cell URI when the replies are sent?
Yes, because LSP client doesn't recognize the "local filesystem URI". LSP server need to translate back to the notebook cell URI and position.
@tanishiking I've had to do something similar here to allow for a simple jar filesystem and translate something like metalsfs:/some-source-jar.jar
<-> file:///some-horrendously-long-path/some-source-jar.jar
See textDocument/implementation as an example
All messages get their URIs translated in and out but it's only file-to-file rather than file-to-multiple-cells.
If notebook cells need URIs translated on (nearly) every message then maybe we should consider the needs of this feature request and https://github.com/scalameta/metals/pull/4114# together.
The other option for the jar file system is this PR which creates a java filesystem for the metalsfs
URI scheme. The same could be done for a vscode-notebook-cell
uri scheme.
This makes the code a lot tidier as no mapping of URIs is needed within the message handling as the java file system can now handle these URIs directly and AbsolutePath can wrap those Paths. I'm not sure how multiple cells would be mapped to one file.
I thought this option was a lot cleaner as it didn't interfere with the main codebase as much but my implementation was slow and I got bogged down in URLClassLoader
@tanishiking I'm not sure how relevant this is to the discussion, but I had a look at almond 0.13.1 (which is awesome) and following the instruction here https://jupyterlab-lsp.readthedocs.io/en/latest/Configuring.html with metals 0.11.8, produced this screenshot;
Does anyone know - is that screenshot proof that juypter is is talking to metals LSP? Or how I might be able to tell?
Och, that's interesting. I wonder if they translate all the cells to a single document automatically :thinking: Maybe we could copy over their solution?
@alexarchambault did you maybe work on that at any point?
It looks like standard completion (the Ammonite one). LSP isn't involved.
Interesting! but jupyter-lsp
shouldn't work with Metals out of the box because
jupyter-lsp
concatenate notebook cells on the frontend which makes usually generates invalid Scala code
jupyter-lsp
uses microsoft/lsp-notebook-concat. This library just concatenates cells but it doesn't work for Scala (especially for Scala2 because top-level definition is invalid)jupyter-lsp
create a concatenated virtual temporary file, the Metals server cannot recognize the file without build information.the notebook cells are concatenated into a single temporary (“virtual”) document on the frontend, which is then sent to the backend, ... as a workaround for some language servers requiring actual presence of the file on the filesystem (against the LSP spec, but common in some less advanced servers), our backend Jupyter server extension creates a temporary file on the file system (by default in the .virtual_documents directory); this is scheduled for deprecation,
added design doc for this feature https://github.com/scalameta/metals/issues/4434
Is your feature request related to a problem? Please describe.
Currently, when using notebook cells in VS Code, Metals is unable to provide any language support. Most likely connected with an exception:
Describe the solution you'd like
Support all the standard language features. We would most likely also need to provide some notebook specific features (such as using values from other cells?).
Describe alternatives you've considered
None I can think of.
Additional contex
This came up in https://github.com/scalameta/metals/discussions/3213 and is separate to supporting https://code.visualstudio.com/api/extension-guides/notebook
Search terms
notebooks vscode