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

Do not cache active interpreter controller in notebookControllerManager #7302

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

joyceerhl
Copy link
Contributor

Fix #7301

Root cause is we were caching the controller corresponding to the active interpreter and never clearing it out when the interpreter changes. I don't think we should be caching the active interpreter controller in the controllerManager because it's already cached in the pythonApi (and the pythonApi class also already bears responsibility for clearing out its cache on an interpreter change), so we would be duplicating the same code in two spots for no real benefit.

Also fixed a bug with our logic to preload the kernel corresponding to the active interpreter. It was always preloading the interactive controller, not the jupyter-notebook controller.

@joyceerhl joyceerhl requested a review from a team as a code owner August 26, 2021 01:22
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2021

Codecov Report

Merging #7302 (17cc7cc) into main (a954838) will decrease coverage by 0%.
The diff coverage is 62%.

@@          Coverage Diff          @@
##            main   #7302   +/-   ##
=====================================
- Coverage     62%     62%   -1%     
=====================================
  Files        360     360           
  Lines      22535   22530    -5     
  Branches    3402    3401    -1     
=====================================
- Hits       14077   14070    -7     
  Misses      7282    7282           
- Partials    1176    1178    +2     
Impacted Files Coverage Δ
.../datascience/notebook/notebookControllerManager.ts 84% <62%> (-1%) ⬇️
src/client/datascience/activation.ts 86% <0%> (-6%) ⬇️
src/client/common/cancellation.ts 72% <0%> (-4%) ⬇️
...t/datascience/notebook/vscodeNotebookController.ts 78% <0%> (-2%) ⬇️
src/client/datascience/baseJupyterSession.ts 61% <0%> (-1%) ⬇️
src/client/datascience/jupyter/jupyterNotebook.ts 21% <0%> (+<1%) ⬆️
src/client/datascience/raw-kernel/rawSocket.ts 84% <0%> (+1%) ⬆️
...tascience/jupyter/kernels/kernelCommandListener.ts 58% <0%> (+2%) ⬆️
src/client/common/application/applicationShell.ts 22% <0%> (+4%) ⬆️

@joyceerhl joyceerhl merged commit a859426 into main Aug 26, 2021
@joyceerhl joyceerhl deleted the dev/joyceerhl/active-interpreter branch August 26, 2021 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive window not started with active interpreter after changing active interpreters
3 participants