Use jupyter inspect to get signature of dynamic functions in notebook editor when language server doesn't provide enough hint. #13259

thy09 · 2020-08-04T08:55:22Z

For #13067, we try to get the signature help from Jupyter kernel for a function which is dynamically created in Jupyter notebook, by calling activeNotebook.inspect like what provideJupyterCompletionItems does.

In the code, when handling the hover request/signature help request, if the following conditions are satisfied, we will show the inspect result from Jupyter Kernel:

The Jupyter kernel provide a callable signature;
The language server doesn't return a callable signature, or the signature of callable is (*args, **kwargs);

Result:
In most cases, the hover and the signature help work as before.
For the case the the function is dynamically created like the following:

def create_function_with_parameters(f, sig, name, doc):
    f = types.FunctionType(
        code=f.__code__,
        globals=f.__globals__,
        name=name,
        argdefs=f.__defaults__,
        closure=f.__closure__,
    )
    f.__signature__ = sig
    f.__doc__ = doc
    return f
signature = Signature([Parameter(name='str_param', kind=Parameter.KEYWORD_ONLY, default='a', annotation=str)], return_annotation=int)
g = create_function_with_parameters(f, sig=signature, name='g', doc="Some doc")

Before:
Signature help:

Hover:

After:
Signature help:

Hover:

…unctions.

codecov-commenter · 2020-08-04T09:04:40Z

Codecov Report

Merging #13259 into master will increase coverage by 0.09%.
The diff coverage is 7.40%.

@@            Coverage Diff             @@
##           master   #13259      +/-   ##
==========================================
+ Coverage   59.56%   59.66%   +0.09%     
==========================================
  Files         669      670       +1     
  Lines       36787    37276     +489     
  Branches     5181     5286     +105     
==========================================
+ Hits        21914    22240     +326     
- Misses      13778    13905     +127     
- Partials     1095     1131      +36

Impacted Files	Coverage Δ
src/client/datascience/jupyter/jupyterNotebook.ts	`4.25% <0.00%> (ø)`
...tascience/jupyter/liveshare/hostJupyterNotebook.ts	`8.02% <0.00%> (ø)`
src/client/datascience/types.ts	`100.00% <ø> (ø)`
...active-common/intellisense/intellisenseProvider.ts	`33.07% <7.59%> (-4.66%)`	⬇️
src/client/common/utils/platform.ts	`64.70% <0.00%> (-11.77%)`	⬇️
src/client/datascience/crossProcessLock.ts	`79.41% <0.00%> (-11.77%)`	⬇️
src/client/activation/common/activatorBase.ts	`13.22% <0.00%> (-9.00%)`	⬇️
src/client/common/terminal/activator/index.ts	`80.95% <0.00%> (-4.77%)`	⬇️
src/client/datascience/jupyter/jupyterExecution.ts	`44.68% <0.00%> (-3.77%)`	⬇️
src/client/datascience/jupyter/jupyterSession.ts	`73.68% <0.00%> (-3.43%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d064494...a035c36. Read the comment docs.

DonJayamanne · 2020-08-04T15:46:32Z

@thy09 thanks for submitting this PR.
However I noticed that you submitted this PR for an issue that was closed.
As a general word of advice, its best to submit PRs for issues that are open and also when the appropriate label as been assigned to the issue (please see guidance here https://github.com/microsoft/vscode-python/blob/master/CONTRIBUTING.md#prerequisites).

FYI - I have re-opened the issue so we can discuss this.

rchiodo · 2020-08-04T17:11:08Z

@thy09 I just wanted to make it clear, this solution is temporary. Once we move to use the VS Code notebook API, this will no longer work as the intellisenseProvider you modified won't be used anymore.

jakebailey · 2020-08-04T17:57:45Z

src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts

+        let signatures: SignatureInformation[] = [];
+        if (callable) {
+            // tslint:disable-next-line:no-object-literal-type-assertion
+            const signatureInfo = <SignatureInformation>{


Why not const signatureInfo: SignatureInformation = ...? (Or just in the below assignment as a literal)

I just learn from this line before: https://github.com/microsoft/vscode-python/blob/master/src/client/providers/signatureProvider.ts#L88

Updated according to your suggestion, thanks a lot.

jakebailey · 2020-08-04T18:00:43Z

src/client/datascience/interactive-common/intellisense/intellisenseProvider.ts

+            const lsHover = lsResult ? convertToMonacoHover(lsResult) : undefined;
+            // If lsHover is not valid or it is not a callable with hints,
+            // we prefer to use jupyterHover which provides better callable hints from jupyter kernel.
+            const preferJupyterHover = !lsHover || !this.isCallableWithGoodHint(lsHover.contents[0].value);


How does this affect tooltips for things that aren't functions, like variables or modules?

Good point!
In fact only callables like functions and methods will get the correct jupyter hover,
so normal variables and modules will still prefer to use language server result.

To make the logic more clear, I updated the logic here,
we prefer jupyter only when the language server doesn't provide callable hover
while jupyter provides callable hover.
Thanks for your suggestion!

…Provider.ts to use notebook.inspect for Hover message and SignatureHelp when PyLance doesn't provide good result.

sonarqubecloud · 2020-08-05T03:10:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 5 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

thy09 · 2020-08-05T03:26:38Z

@thy09 I just wanted to make it clear, this solution is temporary. Once we move to use the VS Code notebook API, this will no longer work as the intellisenseProvider you modified won't be used anymore.

Thanks for your reminder!

When will we move to use the notebook API?
Will this dynamically created function supported with the notebook API?

This is an important feature for our team, just want to make sure how long could we use this feature.
Thanks a lot!

rchiodo · 2020-08-05T16:12:28Z

@thy09 I just wanted to make it clear, this solution is temporary. Once we move to use the VS Code notebook API, this will no longer work as the intellisenseProvider you modified won't be used anymore.

Thanks for your reminder!

When will we move to use the notebook API?

This is actively happening now. In the next few months we plan on deprecating the old way of opening notebooks.

Will this dynamically created function supported with the notebook API?

Not the way you have it now. It would be possible to reimplement this but requires multiple teams to figure out how (see the e-mail stream about pylance). In fact it may turn out completely different. I think it might help to log a feature request on pylance for this work. (https://github.com/microsoft/pylance-release/issues)

This is an important feature for our team, just want to make sure how long could we use this feature.
Thanks a lot!

Hopefully we can get something like this to work in the new notebook api.

thy09 · 2020-08-06T03:58:42Z

@thy09 I just wanted to make it clear, this solution is temporary. Once we move to use the VS Code notebook API, this will no longer work as the intellisenseProvider you modified won't be used anymore.

Thanks for your reminder!
When will we move to use the notebook API?

This is actively happening now. In the next few months we plan on deprecating the old way of opening notebooks.

Will this dynamically created function supported with the notebook API?

Not the way you have it now. It would be possible to reimplement this but requires multiple teams to figure out how (see the e-mail stream about pylance). In fact it may turn out completely different. I think it might help to log a feature request on pylance for this work. (https://github.com/microsoft/pylance-release/issues)

This is an important feature for our team, just want to make sure how long could we use this feature.
Thanks a lot!

Hopefully we can get something like this to work in the new notebook api.

Thanks for you kind reply!
The feature request is created: microsoft/pylance-release#204

Heyi Tang added 3 commits July 30, 2020 18:46

Update the signature for jupyterNotebook.inspect to support inspect f…

898369c

…unctions.

Update interface signature of INotebook.inspect in types.ts

abb0ed8

Update affected calls of INotebook.inspect.

9d3d7bc

thy09 changed the title ~~Thy09/use jupyter inspect to get signature of dynamic functions when language server doesn't provide enough hint.~~ Use jupyter inspect to get signature of dynamic functions when language server doesn't provide enough hint. Aug 4, 2020

thy09 changed the title ~~Use jupyter inspect to get signature of dynamic functions when language server doesn't provide enough hint.~~ Use jupyter inspect to get signature of dynamic functions in notebook editor when language server doesn't provide enough hint. Aug 4, 2020

rchiodo approved these changes Aug 4, 2020

View reviewed changes

rchiodo requested review from DonJayamanne, DavidKutu, greazer, IanMatthewHuff and joyceerhl August 4, 2020 16:27

IanMatthewHuff approved these changes Aug 4, 2020

View reviewed changes

jakebailey reviewed Aug 4, 2020

View reviewed changes

DavidKutu approved these changes Aug 4, 2020

View reviewed changes

thy09 force-pushed the thy09/use-jupyter-inspect-to-get-signature-of-dynamic-functions branch from 4f04079 to cff8239 Compare August 5, 2020 02:54

Heyi Tang added 2 commits August 5, 2020 11:04

Add news entry

d902efe

Add logic in datascience/interactive-common/intellisense/intellisense…

a035c36

…Provider.ts to use notebook.inspect for Hover message and SignatureHelp when PyLance doesn't provide good result.

thy09 force-pushed the thy09/use-jupyter-inspect-to-get-signature-of-dynamic-functions branch from cff8239 to a035c36 Compare August 5, 2020 03:08

rchiodo merged commit 44fd2d8 into microsoft:master Aug 5, 2020

thy09 mentioned this pull request Aug 6, 2020

Talk to Jupyter kernel/VSCode Notebook kernel to support dynamically created function. microsoft/pylance-release#204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use jupyter inspect to get signature of dynamic functions in notebook editor when language server doesn't provide enough hint. #13259

Use jupyter inspect to get signature of dynamic functions in notebook editor when language server doesn't provide enough hint. #13259

thy09 commented Aug 4, 2020 •

edited

Loading

codecov-commenter commented Aug 4, 2020 •

edited

Loading

DonJayamanne commented Aug 4, 2020

rchiodo commented Aug 4, 2020

jakebailey Aug 4, 2020 •

edited

Loading

thy09 Aug 5, 2020

jakebailey Aug 4, 2020

thy09 Aug 5, 2020 •

edited

Loading

sonarqubecloud bot commented Aug 5, 2020

thy09 commented Aug 5, 2020

rchiodo commented Aug 5, 2020

thy09 commented Aug 6, 2020

Use jupyter inspect to get signature of dynamic functions in notebook editor when language server doesn't provide enough hint. #13259

Use jupyter inspect to get signature of dynamic functions in notebook editor when language server doesn't provide enough hint. #13259

Conversation

thy09 commented Aug 4, 2020 • edited Loading

codecov-commenter commented Aug 4, 2020 • edited Loading

Codecov Report

DonJayamanne commented Aug 4, 2020

rchiodo commented Aug 4, 2020

jakebailey Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

thy09 Aug 5, 2020

Choose a reason for hiding this comment

jakebailey Aug 4, 2020

Choose a reason for hiding this comment

thy09 Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 5, 2020

thy09 commented Aug 5, 2020

rchiodo commented Aug 5, 2020

thy09 commented Aug 6, 2020

thy09 commented Aug 4, 2020 •

edited

Loading

codecov-commenter commented Aug 4, 2020 •

edited

Loading

jakebailey Aug 4, 2020 •

edited

Loading

thy09 Aug 5, 2020 •

edited

Loading