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

Remove Python ext API registerGetNotebookUriForTextDocumentUriFunction and move code into Python/Pylance extension #14977

Closed
DonJayamanne opened this issue Jan 4, 2024 · 10 comments · Fixed by #15465 · May be fixed by #15244
Assignees
Labels
debt Code quality issues notebook-intellisense Intellisense & other language features in notebook cells for any language
Milestone

Comments

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Jan 4, 2024

Given that

  • Interactive window is a core feature in VS Code,
  • and all of the code used to get the Notebook Uri for the above function is not relying on any Jupyter ext,

its simpler (for maintenance) to move this code into Python/Pylance extension.

This is one less API in Python extension.

Thoughts/Concerns ? @karthiknadig @amunger @debonte

@DonJayamanne DonJayamanne added notebook-intellisense Intellisense & other language features in notebook cells for any language debt Code quality issues labels Jan 4, 2024
@DonJayamanne DonJayamanne self-assigned this Jan 4, 2024
@DonJayamanne DonJayamanne changed the title Move registerGetNotebookUriForTextDocumentUriFunction into Python/Pylance extension Move Python ext API registerGetNotebookUriForTextDocumentUriFunction into Python/Pylance extension Jan 4, 2024
@DonJayamanne DonJayamanne changed the title Move Python ext API registerGetNotebookUriForTextDocumentUriFunction into Python/Pylance extension Remove Python ext API registerGetNotebookUriForTextDocumentUriFunction and move code into Python/Pylance extension Jan 4, 2024
@amunger
Copy link
Contributor

amunger commented Jan 4, 2024

I imagine this would helped by microsoft/vscode#154983 or even made unnecessary.

@DonJayamanne
Copy link
Contributor Author

I do not believe that would help,
Neither Pylance nor Python extension will not be able to use that API to determine whether a given URI is an interacrtive URI belonging to a particular notebook or not.

@debonte
Copy link
Contributor

debonte commented Jan 18, 2024

I started trying to move this code into Pylance and found that it calls isInteractiveInputTab which uses vscode-jupyter's InteractiveTab and TabInputInteractiveWindow types. Are the structures of these types well-defined? It looks like maybe they are vscode types that vscode-jupyter duplicated?

@DonJayamanne
Copy link
Contributor Author

Are the structures of these types well-defined? It looks like maybe they are vscode types that vscode-jupyter duplicated?

Hmm, yes you are right, perhaps @amunger might have some idea why we copied the types.
I believe you could either copy the types (as done here) or just add the proposed API into pylance to just get the type definitions (and ensure there's a very strict contract, to catch changes to the types).

@debonte debonte self-assigned this Jan 18, 2024
@debonte
Copy link
Contributor

debonte commented Jan 22, 2024

I've removed the API from Pylance, so Pylance will no longer be relying on Python/Jupyter to provide a function to resolve the input box URI to a notebook URI. We're aiming to ship a prerelease build today (2023.12.104) that will include this change and we should be shipping a stable/release build in the next week or so.

I think once Pylance's stable build includes my change it should be safe for Python and Jupyter to remove their related code. Anyone that hits compatibility issues (ex. new Python + old Pylance) can be told to upgrade Pylance. @DonJayamanne, @karthiknadig, do you agree?

cc: @luabud

@karthiknadig
Copy link
Member

@debonte Yes, it makes sense me. for compatibility we could also set the engine number so that the right versions match for pylance/python/jupyter.

@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented Jan 28, 2024

@debonte thanks, I will prepare a PR to remote this API from Jupyter extension (but will wait for Pylance to first ship befmore merging this PR)

Any idea when the next stable version goes out? is it lined up with the Python/vscode release schedule?

@DonJayamanne DonJayamanne added this to the February 2024 milestone Jan 28, 2024
@debonte
Copy link
Contributor

debonte commented Jan 30, 2024

Any idea when the next stable version goes out? is it lined up with the Python/vscode release schedule?

Likely sometime this week. We are not in strict alignment with the vscode release schedule.

@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented Feb 27, 2024

I will be removing the registerGetNotebookUriForTextDocumentUri function from Jupyter extension
The corresponding code from Python extension will need to go, however that impacts the following tests as well src/test/activation/node/lspInteractiveWindowMiddlewareAddon.unit.test.ts

Not sure where these tests should live nor whether it requires a change.
Leaving that for you @debonte as you wrote those tests in the Python extension

@debonte
Copy link
Contributor

debonte commented Feb 27, 2024

The LspInteractiveWindowMiddlewareAddon lives in Pylance now. It was moved over there about a year ago as part of our client refactoring work. But we left the old implementation behind in vscode-python for users on older Pylance builds. I think it's safe for vscode-python to remove this now. PR is here: microsoft/vscode-python#22985

debonte added a commit to microsoft/vscode-python that referenced this issue Feb 29, 2024
Part of microsoft/vscode-jupyter#14977

Removes some effectively dead code that would only be used by very old
(year+) versions of Pylance. This logic has all been moved into
vscode-pylance:

- Middleware to make the interactive window input box look like a
notebook cell
- Call to Jupyter to get pythonpath for notebooks.
- Configuration hook to get `[python]` value of `editor.formatOnType`.
- LSP notebook experiment
@DonJayamanne DonJayamanne modified the milestones: March 2024, April 2024 Mar 26, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues notebook-intellisense Intellisense & other language features in notebook cells for any language
Projects
None yet
4 participants