Conversation
Fixes pappasam#324.
|
@pappasam this is ready for review |
|
Looks like the tests are failing on Windows because of line endings |
|
@pappasam this is ready for review again. I looked into the Windows issue and it appears to be caused by an unrelated bug in how pygls handles line endings versus how parso does (#159 (comment)). I've skipped the test on Windows for now. |
There was a problem hiding this comment.
Thank you for spending the time crafting this thorough PR! Support for .ipynb will definitely be a killer feature.
Responses
- I agree with your decision to stay away from
jedi_utils, especially during this early stage. We can always generalize later. - I think it's worth exploring a limit on the number of concurrent diagnostics requests, either globally or for each notebook. I'd prefer that we slow things down instead of reducing capabilities.
Architectural Thoughts
One concern I have is how we're considering both .py and .ipynb dynamically for each server endpoint. We're adding filetype checking logic to each endpoint, which complicates the code.
Might it be better to duplicate server.py into something like server_ipynb.py and provide either a command line flag (--target notebook/script) or a different binary (jedi-language-notebook-server...)? A project with both notebooks and scripts might need two server instances, but this would keep implementations separate and provide a cleaner path for supporting additional filetypes.
Still thinking through this one, but putting these thoughts here to spur discussion if you've considered these points already.
Testing Request
Could you provide:
- A simple example notebook
- Steps for setting up a basic Jupyter environment
- Instructions for configuring an editor to use our language server with notebooks
Since I use Neovim, I'd likely need to test through the notebook server in a browser, so any guidance there would be especially helpful.
|
Thank you for the feedback! Here's how you can test the branch in an editor:
You can try out any of the test notebooks used in this branch. The main thing is testing that things work across cells. |
@pappasam I agree that the code is complicated by the script/notebook decision spread over too many places. Although I think we can address that without requiring an additional server. We could add a I doubt that the dispatching would add any real performance overhead. What do you think?
I suspect that |
Yes, I like this approach! Only clarification I'd like to make here: |
|
Exciting progress! I finally spun up code to test and noticed a couple things: First: my (albeit underpowered) laptop is on fire With code+notebook open: Once code is closed: Second: It looks like Goto Definition doesn't work for variables that are defined in cells (it does work for things installed in the virtualenv): In fact, it looks like cross-cell references don't really work. Do you have a different experience? If yes, maybe you can share a video demonstrating successful usage of the various features. My system: |
|
That's unfortunate! Can you try on 936f4a9? I might've broken something while refactoring. I'll take a look at the CPU usage, fix any bugs that might've crept in, and post a video on Monday. Does CPU usage only start when the language server is busy in a notebook or could that be related to something else in VSCode? |
|
@pappasam Sorry about that, I had introduced a bug while refactoring. It should work on the latest commit, here's a video: Screen.Recording.2025-03-11.at.13.44.44.movNote that if the Python extension updated, you'll have to change the path you
Please take a look and let me know what you think 😄 |
pappasam
left a comment
There was a problem hiding this comment.
Sorry for the late response, I was traveling and finally found time to devote to this.
- The performance issues don't seem to be affecting with your latest commits! Either it was a spurious vs-code issue, or your fixes did the trick.
- Overall, I've manually tested various features, and things appear to work great (goto definition successfully navigatest to appropriate cells, python files open, auto-completion works, etc).
One question for you inline regarding the semaphore.
All-in-all, this looks like it's just about ready to go!
jedi_language_server/jedi_utils.py
Outdated
| func(*args, **kwargs) | ||
| _debounce_semaphore.release() |
There was a problem hiding this comment.
Any reason we're not wrapping this in a try/finally? Haven't fully wrapped my head around the implication, but it feels like we might want to release the semaphore even if an exception occurs in func.
Yeah, we should probably disable variable/function code actions for notebooks. Note: after seeing it, I don't hate the decorator approach you've designed and implemented here |
|
Thanks for the review! I've wrapped the semaphore release and disabled code actions for notebooks.
Cool, let's go with this approach for now since I'm also running out of time on this. If we ever revisit this in future, I saw a middleware approach in the JS language client that might be more generally useful. The idea is to be able to register a middleware class that implements methods like: def completion(self, server, params, next):Where |
pappasam
left a comment
There was a problem hiding this comment.
🚀
Amazing, thanks so much for contributing this feature, and for all the thought you've put into both the current design and next steps




This PR adds notebook support. Fixes #324.
How it works
textDocument/*request is for a notebook cell.Unchanged language features
The following methods are excluded because the full notebook source shouldn't affect the result of a given cell:
textDocument/documentSymboltextDocument/highlightUnsupported cases
There are also a few methods which work well if the source of the request is from a notebook cell, but ignore any other
.ipynbfiles. I think the root of this issue is that Jedi doesn't support.ipynbfiles:textDocument/referencestextDocument/renameworkspace/symbolQuestions
Possible diagnostics performance concern
I decided to publish diagnostics independently for each cell. This means we might start many threads when opening a large notebook. Do you think that's a concern?
It should be less of an issue for changing notebooks, since I don't think many cells should change in a short period of time.
We could instead:
Package for use by other pygls servers?
I ended up keeping
jedi_utilsas untouched as possible to keep the existing decoupling, and adding most of the new logic intonotebook_utils. This ends up being quite verbose inserverbut I think it keeps things much simpler overall. What do you think?I think it might be possible to totally decouple this approach from jedi-language-server, either making it a separate package, or as a PR to pygls. Kind of like a decorator over LSP feature functions that modifies
document.sourceand any positions/ranges inparams, and similarly for the output. The benefit would be that it could be used in other pygls servers too. I haven't given it too much thought though, so there might be some hard blockers there.