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

Less aggressive inspect request for completer ? #15787

Open
Carreau opened this issue Feb 12, 2024 · 3 comments
Open

Less aggressive inspect request for completer ? #15787

Carreau opened this issue Feb 12, 2024 · 3 comments

Comments

@Carreau
Copy link
Contributor

Carreau commented Feb 12, 2024

It seem that recently (?) the completer was modified to send inspect request when cycling through completions.

This leads to some problems, as some extensions for object inspect (deshaw/pyflyby#296) assume that the objects exists, and this leads to the jupyterLab completer triggering side effects (imports).

Would you be open to splitting the current ipykernel inspects requests (or add a parameter), to distinguish requests for objects that might, or might not exists.

This would likely require coordination between likely a few packages to add new APIs.

@Carreau Carreau added enhancement status:Needs Triage Applied to new issues that need triage labels Feb 12, 2024
@krassowski
Copy link
Member

Completer should only send inspect requests when showDocumentationPanel option is enabled. If this is not the case, then there is a bug.

Would you be open to splitting the current ipykernel inspects requests (or add a parameter), to distinguish requests for objects that might, or might not exists.

What does it mean that an object exists? AFAIK the inspect request can be also sent on arbitrary text when moving cursor with the inspector panel open, right?

@Carreau
Copy link
Contributor Author

Carreau commented Feb 12, 2024

As far as I can tell it also send request just when cycling, I will recheck.

I agree that it's a bit on the edge about this request as no-one is doing something obviously wrong. The problem with sending inspect request with the completer is that it send requests for text that is not in a cell and rapid succession. Inspect was originally ment to pull info for ? and ?? which should not work if the object doe not exists. But at the same time if someone explicitly say numpy? it's obvious they want to import numpy, and the interaction of the both leads to nu<tab> importing numpy.

@krassowski
Copy link
Member

As far as I can tell it also send request just when cycling, I will recheck.

From a quick look around the codebase it seems that it is indeed what is happening, and fixing it is non-trivial (without breaking downstream).

The resolve method does not have awareness of the completer widget state (whether docs panel is shown):

async resolve(
item: CompletionHandler.ICompletionItem,
context: ICompletionContext,
patch?: Completer.IPatch | null
): Promise<CompletionHandler.ICompletionItem> {

Adding an argument to the inspect request sounds good. Of note we already set detail_level to 0:

const contents: KernelMessage.IInspectRequestMsg['content'] = {
code,
cursor_pos: offset,
detail_level: 0
};
const msg = await kernel.requestInspect(contents);

We previously talked about having an argument to completion request to enable completer with LSP to say "I want only the completions derived from dynamic rather than static analysis because for static analysis I can do a better job". It looks like this is a very similar scenario where we want only static analysis (no imports) - what do you think?

@JasonWeill JasonWeill changed the title Less agressive inspect request for completer ? Less aggressive inspect request for completer ? Feb 13, 2024
@JasonWeill JasonWeill removed the status:Needs Triage Applied to new issues that need triage label Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants