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/merge preferred controller, default controller, loader, registration, etc #11914

Closed
rebornix opened this issue Nov 4, 2022 · 5 comments · Fixed by #11944
Closed

Remove/merge preferred controller, default controller, loader, registration, etc #11914

rebornix opened this issue Nov 4, 2022 · 5 comments · Fixed by #11944
Assignees
Labels
Milestone

Comments

@rebornix
Copy link
Member

rebornix commented Nov 4, 2022

Currently we have both controllerDefaultService and controllerPreferredService, which have overlap but differ a bit at the same time.

@rebornix rebornix mentioned this issue Nov 4, 2022
54 tasks
@rebornix rebornix added the debt Code quality issues label Nov 4, 2022
@rebornix rebornix added this to the November 2022 milestone Nov 4, 2022
@rebornix
Copy link
Member Author

rebornix commented Nov 4, 2022

While we consolidate these two services, we should probably also simply how we create default/preferred controller, which currently is based on some heuristics, including:

  • whether it's localOnly
  • whether there is any kernel created (when there is an active python intepreter)
  • whether we are connected to a remote (traditional flow)

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Nov 7, 2022

  • ControllerDefaultService -> Determines the 'default' kernel for a notebook. Default is what kernel should be used if there's no metadata in a notebook.
  • ControllerPreferredService -> Computes and tracks the preferred kernel for a notebook. Preferred is determined from the metadata in the notebook. If no metadata is found, the default kernel is used.
  • KernelRankingHelper.isExactMatch -> This seems to doe both of the above items as well!
  • IW - We create the controller for active interrpeter always
  • CibtrikkerKiader - we create active interpreter controller
  • notebookKernelSourceTracker - we show/hide controllers per notebook
  • controllerPreferredService, vscodeNotebookController and notebookKernelSourceTracker
    • All three of these update the affinity of a controller with a notebook
  • Also found that the methdo findPreferredKernelExactMatch doesn't find the exact match, instead this finds a close match.
  • Stop using preferred and defaujlt service, this is only used in old code paths and IW and IW debugger &oh yes tests.

@DonJayamanne DonJayamanne reopened this Nov 11, 2022
@DonJayamanne DonJayamanne changed the title Remove/merge preferred controller, default controller Remove/merge preferred controller, default controller, loader, registration, etc Nov 14, 2022
@DonJayamanne
Copy link
Contributor

DonJayamanne commented Nov 14, 2022

Next to do item,

  • KernelRankingHelper.isExactMatch -> This seems to doe both of the above items as well!

@DonJayamanne
Copy link
Contributor

Most of the work is done, we'll need to wait for the new Kernel Picker to be stable, and once we delete the old code, then reviewing the existing code/structure would be simpler

@DonJayamanne
Copy link
Contributor

Marking as closed

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants