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

Unify remote kernel finder with ServerUriStorage #11645

Closed
Tracked by #12832
rebornix opened this issue Oct 13, 2022 · 4 comments · Fixed by #13725 or #13752
Closed
Tracked by #12832

Unify remote kernel finder with ServerUriStorage #11645

rebornix opened this issue Oct 13, 2022 · 4 comments · Fixed by #13725 or #13752
Assignees
Labels
debt Code quality issues notebook-remote Applies to remote Jupyter Servers on-testplan
Milestone

Comments

@rebornix
Copy link
Member

With the introduction of the new remote finder and server picker experience, we might want to revisit how we are registering kernels from remote uris and what we store in ServerUriStorage

  • ServerUriStorage can contain following info
    • url list
      • user provided server urls
      • third party contributed server urls
      • local when users explicitly set the server to None from the server picker
    • currentServerId referring to the currently picked entry
    • localOnly referring to if we should connect to local only (❓)
  • Traditional workflow
    • LocalKernelFinder contributes all local kernels
    • RemoteKernelFinder checks ServerUriStorage#currentServerId and decides if/how to detect kernel specs/sessions
      • If the selected entry in ServerUriStorage is local, then it doesn't attempt to scan kernel specs/sessions.
  • New workflow
    • UniversalRemoteKernelFinderController creates a UniRemoteKernelFinder for all entries
    • Each UniRemoteKernelFinder follows the same logic as RemoteKernelFinder, if its uri is local, it contributes empty kernel specs/sessions.

It would be great if we can move local out of the ServerUriStorage and we can simply the RemoteKernelFinder ctor and registrations into

  • ServerUriStorage only holds server uri infos
  • Traditional remote kernel finder controller
    • Hold the currentSelectedUri info
    • Register/dispose remote kernel finder per currentSelectedUri update
  • New remote kernel finder controller
    • Register/dispose all remote kernel finders per ServerUriStorage#getUris

cc @IanMatthewHuff @DonJayamanne

@IanMatthewHuff
Copy link
Member

Yeah, the current usage of ServerUriStorage is hacky given how I use it from the new experimental picker. I'm actually kinda intrigued on a concept like the ContributedKernelFinder moving up into the UI layer. From the perspective of the picker I wouldn't want to have to care about the difference between server / local / whatever. All that really matters is there is some thing X that provides kernels. Maybe a UriStorage isn't needed at all? Instead we are just working with a list of KernelSources some are local, or remote, or third party, ect... If something is added to the list it can control its own registration for kernel finding. So you could do something like an extensibility point that let you add kernels directly from another extension, instead of adding servers.

@rebornix
Copy link
Member Author

Instead we are just working with a list of KernelSources some are local, or remote, or third party, ect... If something is added to the list it can control its own registration for kernel finding.

We can consider moving off ServerUriStorage and store connection type/info in each ContributedKernelFinder instance and we store the list in memento. On window reload, we then create a list of ContributedKernelFinders from the cached list.

@DonJayamanne DonJayamanne self-assigned this Nov 28, 2022
@DonJayamanne DonJayamanne added the debt Code quality issues label Dec 4, 2022
@DonJayamanne
Copy link
Contributor

@rebornix lets have a chat about this, I think this is related to Refactor/simplify Jupyter Connection here #11904

@DonJayamanne
Copy link
Contributor

  • Write to old format and the new storage format
  • If users run into issues they opt out of experiment (new stuff is still in old storage as well)
  • Experiment to use either one storage formats

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues notebook-remote Applies to remote Jupyter Servers on-testplan
Projects
None yet
3 participants