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

Explore how to simplify the code for managing connections to local and remote Jupyter Servers #12832

Closed
17 tasks done
DonJayamanne opened this issue Feb 13, 2023 · 2 comments · Fixed by #13555 or #14393
Closed
17 tasks done
Assignees
Labels
debt Code quality issues
Milestone

Comments

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Feb 13, 2023

Tasks

  1. debt
    DonJayamanne
  2. debt
    DonJayamanne
  3. debt on-testplan
    DonJayamanne
  4. debt on-testplan
    DonJayamanne
  5. 4 of 4
    DonJayamanne
  6. debt
    DonJayamanne
  7. author-verification-requested bug info-needed notebook-remote verified
    DonJayamanne
  8. bug info-needed notebook-remote
    DonJayamanne
  9. bug info-needed notebook-remote
    DonJayamanne
  10. bug notebook-kernel-remote notebook-remote verified
    DonJayamanne
  11. bug info-needed notebook-remote remote-connection-failure verified
    DonJayamanne
  12. debt notebook-kernel-remote notebook-remote on-testplan
    DonJayamanne
  13. debt notebook-kernel-remote notebook-remote
    DonJayamanne
  14. debt
    DonJayamanne
  15. debt
    DonJayamanne
  16. debt notebook-kernel-remote
    DonJayamanne
  17. debt notebook-kernel-remote perf
    DonJayamanne
@DonJayamanne DonJayamanne added bug Issue identified by VS Code Team member as probable bug debt Code quality issues labels Feb 13, 2023
@DonJayamanne DonJayamanne self-assigned this Feb 13, 2023
@DonJayamanne DonJayamanne added this to the May 2023 milestone Apr 28, 2023
@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented May 22, 2023

Problems

Ambiguous Uri, Url, ServerId #13540
  • The term Uri is used in a number of places to point to the address of the Jupyter Server as well as an identifyer for a Jupyter Server.
    • Without inspecting the Uri we have no idea what it is, hence we have if conditions in a number of places
    • Unfortunately there are places where this if condition isn't used and has caused bugs (password storage and connection)
    • Error messages and the like assume the Url is the Jupyter Server Uri, but its not.
    • Basically its just too complex
  • The Uri is constructed from the Jupyter Provider Id and Handle, however without the extensionId it is not unique.
    • Its possible to have two extensions with the same provider id, and this can cause issues when we try to connect to them.
  • The Uri is constructed from the Jupyter Provider Id and Handle
    • Hence Uri is kind of always unique (provided we include the extension id into the mix)
    • Thus we do not need computeServerId
    • We have serverId used as identifiers in a number of places, along with this Uri as well
    • We have two types of identifiers

Suggested Solutions:

  • Remove serverId
  • Replace serverId with the Uri
  • Replace Uri with an object, today its called idAndHandle, its basically a Jupyter Server Identifier (lets call this JupyterServerProviderHandle and will include extensionId as a new property
Invalid password not cleared until we start VS Code

Root cause is the same as the previous problem. Uri !== Uri

Server already exists

When attempting to add a new Server sometimes we end up with the error indicating this already exists.
The problem is basically related to the previous issue as well as tech debt.
Root cause is not yet known, but the code is a little too complex and we have too much state (instead of getting the latest state from memento/auth storage).
It seems the in-memory state is stale and users run into this issue.

Suggestions:

  • Remove the cache of _servers or only leave that when adding a new server and then remove that.
    I.e. where possible remove the caches.
No proper connection validation stage when starting a remote kernel

When connecting to a server (or starting a kernel), this is what we do:

  • Create the connection IJupyterConnection
  • Next, start the session with the SessionManager using the IJupyterConnection
    It is at this point that the connection is validated.

Problem:

  • If the password is invalid, we validate that when starting the session, not when creating the connection
  • If some other information is invalid then the same as above

Question:

  • Should we have a separate step when we verify/validate the connection
    • Today we have a separate validation step only when selecting a kernel from a provider (e.g. when adding a Remote Jupyter Uri from the existing kernel Picker)

Suggested Solutions:

  • Have a separate step to validate the connection
    The benefits are that we can easily update the code to incorporate management of the connection, updating the password, etc.
    Else adding all of this into the session creation code/layer seems messy.
Debt: Not waiting for async Session disposal to complete

In JupyterConnection.ts when we dispose the session, we do not wait for it to finish after we've validated the connection information.
Method validateRemoteUri, code sessionManager.dispose().catch(noop);

  • We never clear old cache if a Remote Uri is deleted, this cache key lives forever ${ContributedKernelFinderKind.Remote}-${serverHandleId},

  • If we have a token for a Jupyter Server, then why check and prompt for passwords,
    Both steps are totally unnecessary (more ui and more points of failure).

  • Rename IJupyterSessionManager #13578

  • Export Notebook for Web: call JupyterLab#ContentManager directly to simplify JupyterKernelSession #13577

  • Move the code that creates the ServerConnection.ISettings object from our IJupyterConnection into the Connection layer.
    This is related to how Jupyter Lab connects to the remote servers, has nothing to do with sessions.
    Hence this method should be removed from JupyterSessionManager into the JupyterConnection class.
    E.g. we could add a method JupyterConnection.toJupyterConnectionSessions(IJupyterConnection)

@amunger
Copy link
Contributor

amunger commented Oct 25, 2023

removing from verification queue

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
2 participants