Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experiment sending the complete_request message on the control channel. #11831

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 12, 2022

References

Code changes

With ipykernel 6.0 or later, this enables the complete request to be processed in parallel with execution, so we can still get completions while the kernel is busy.

This is an experiment to see if we can improve autocomplete responsiveness. Luckily, it seems that ipykernel accepts any shell message on the control channel (see ipython/ipykernel@2894558#diff-afe8c3a6fde7c69241bf107a35a82a41da345bcbfa62bb23347ca30f7d25acd8R164 for when this was introduced in 2012), so this should work with any ipykernel version 6 or later. The messaging spec implies that sending completion requests to the control channel is not officially supported.

User-facing changes

In one cell, execute a sleep, like: import time; time.sleep(20)

Immediately after that, while it is still busy, try doing autocomplete (like pri<tab>). The autocomplete should work.

Backwards-incompatible changes

With ipykernel 6.0 or later, this enables the complete request to be processed in parallel with execution, so we can still get completions while the kernel is busy.
@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor Author

@krassowski mentions in the dev meeting that jedi, when executed in threads, can cause huge issues. @krassowski, can you elaborate?

@jasongrout
Copy link
Contributor Author

The Jedi docs include this warning:

By default jedi.settings.fast_parser is enabled, which means that parso reuses modules (i.e. they are not immutable). With this setting Jedi is not thread safe and it is also not safe to use multiple Script instances and its definitions at the same time.
If you are a normal plugin developer this should not be an issue. It is an issue for people that do more complex stuff with Jedi.
This is purely a performance optimization and works pretty well for all typical usages, however consider to turn the setting off if it causes you problems. See also this discussion.

@krassowski
Copy link
Member

In addition to the above, davidhalter/jedi#1807 (comment) is another example. pylsp has a lock on each operation to circumvent some issues, but it prevents it from handling multiple requests at once really.

It might not be an issue for completions on control channel though. The problem would be only if this change would allow to send two completion requests simultaneously for concurrent execution.

@jasongrout-db
Copy link

The problem would be only if this change would allow to send two completion requests simultaneously for concurrent execution.

This could happen if a client sent a completion request on the control channel while another client sent a completion request on the shell channel.

@jasongrout
Copy link
Contributor Author

This could happen if a client sent a completion request on the control channel while another client sent a completion request on the shell channel.

Of course, the easy thing here is to introduce a coordination mechanism in ipykernel between the control and main thread to not process completion messages concurrently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants