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

Wrong kernel name in metadata kernelspec when using kernel from virtualenv #8796

Closed
pplonski opened this issue Jan 25, 2022 · 22 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Milestone

Comments

@pplonski
Copy link

pplonski commented Jan 25, 2022

Environment data

  • VS Code version: 1.63.2
  • Jupyter Extension version (available under the Extensions sidebar): v2021.11.1001550889
  • Python Extension version (available under the Extensions sidebar): v2021.12.1559732655
  • OS (Windows | Mac | Linux distro) and version: Ubuntu 18.04
  • Python: Python 3.8
  • Type of virtual environment used: virtualenv
  • Jupyter server running: Local

Expected behaviour

I created a new virtual environment with the name venv_2.

virtualenv venv_2 --python=python3.8

I add kernel to that venv:

python -m ipykernel install --user --name=venv_2

I created a new ipynb file in the VS Code and select kernel to venv_2 in the top right corner.

When I check the file of the notebook in the text editor I see the wrong kernelspec in metadata, it has name set to python3 but it should be set to venv_2.

The contents of the notebook:

... the rest of the notebook ...
"metadata": {
  "kernelspec": {
   "display_name": "Python 3.8.0 64-bit ('venv_2': virtualenv)",
   "language": "python",
   "name": "python3"
  },

It should be

... the rest of the notebook ...
"metadata": {
  "kernelspec": {
   "display_name": "Python 3.8.0 64-bit ('venv_2': virtualenv)",
   "language": "python",
   "name": "venv_2"
  },
@pplonski pplonski added the bug Issue identified by VS Code Team member as probable bug label Jan 25, 2022
@rchiodo
Copy link
Contributor

rchiodo commented Jan 25, 2022

Thanks for the bug.

This is a side effect of how we start kernels. We don't use the kernelspec you wrote to disk. If there's a virtual environment, we just start the kernel directly ourselves. Which means we never 'find' the name 'venv_2' as the name of the kernel.

Meaning it won't work if you open the same notebook in Jupyter.

@rchiodo rchiodo removed their assignment Jan 25, 2022
@pplonski
Copy link
Author

@rchiodo that's right, it doesn't work if I open the notebook in the Jupyter.

Are you going to fix this?

I'm asking because I've created a framework for converting Jupyter notebooks into web apps by adding YAML header. The framework is open-source https://github.com/mljar/mercury I would like to add an extension to VS Code - to be able to watch the app development in Jupyter & Mercury.

@rchiodo
Copy link
Contributor

rchiodo commented Jan 25, 2022

Are you going to fix this?

This depends upon a lot of things. How many people hit this issue, How often we think people create kernelspecs for venvs vs just installing jupyter in them and running them directly. How hard it would be to fix this.

This is the first complaint of this issue (and it's existed for the last two years) so my honest guess is no this won't be high priority.

I don't think it would be that hard to fix though. It's merely changing the 'name' associated with our internal representation of a kernel. We could look to see if there's a kernelspec that matches a virtual environment and if so, use that as the name instead of our default.

@rchiodo
Copy link
Contributor

rchiodo commented Jan 25, 2022

If you have the time to fix it, we certainly accept PRs as well. The logic is rather messy, but it's probably somewhere here:

We're eliminating duplicates (the venv_2 kernelspec was likely marked as a dupe). In that dupe eliminating code, the kernelspec name might be saved to be used later.

@pplonski
Copy link
Author

@rchiodo do you think that it will be enough to replace following line

kernelSpec.name = kernelJson?.name || path.basename(path.dirname(specPath));

with

kernelSpec.name = kernelJson?.name || kernelJson?.display_name || path.basename(path.dirname(specPath)); 

With this line the code will try to replace the name with display_name which in my case will give desired result. The path.basename(path.dirname(specPath)) will be as fallback.

I cant find tests for loadKernelSpec method, where I can found them?

@rchiodo
Copy link
Contributor

rchiodo commented Jan 25, 2022

Sorry but I don't think that will fix the problem. display_name shouldn't be used for kernelSpec.name (as the name is actually a directory). That line should already be picking up your kernelspec name (which would be venv_2)

I think the problem is later when we remove duplicates.

Tests for this are generally here:
https://github.com/microsoft/vscode-jupyter/blob/cae887e8fed0fed24896236a45fa97cec71c7622/src/test/datascience/notebook/kernelSelection.vscode.test.ts

I don't believe we have a test for this case yet.

@pplonski
Copy link
Author

@rchiodo it makes sense. But I just dont see how removing duplicates can change the name. I've check the

const originalSpecFiles = new Set<string>();
results.forEach((r) => {
if (r.metadata?.originalSpecFile) {
originalSpecFiles.add(r.metadata?.originalSpecFile);
}
});
results = results.filter((r) => !r.specFile || !originalSpecFiles.has(r.specFile));
// There was also an old bug where the same item would be registered more than once. Eliminate these dupes
// too.
const unique: IJupyterKernelSpec[] = [];
const byDisplayName = new Map<string, IJupyterKernelSpec>();
results.forEach((r) => {
const existing = byDisplayName.get(r.display_name);
if (existing && existing.path !== r.path) {
// This item is a dupe but has a different path to start the exe
unique.push(r);
} else if (!existing) {
unique.push(r);
byDisplayName.set(r.display_name, r);
}
});

Maybe there is some other place which change the name?

@rchiodo
Copy link
Contributor

rchiodo commented Jan 25, 2022

The 'interpreter' based kernelspec takes precedence over the local kernel spec one. It uses the 'name' of the interpreter based kernelspec which defaults to python3

@rchiodo
Copy link
Contributor

rchiodo commented Jan 25, 2022

Somewhere in this function it is:

  • Generating kernelspecs for every interpreter on the system
  • Generating kernelspecs for every kernelspec on the system
  • Eliminating dupes

@pplonski
Copy link
Author

@rchiodo thank you, but I give up.

Because of this bug, VS Code Jupyter notebooks cant be run on Jupyter Notebook and Jupyter Lab and with tools like nbconvert. Maybe there were not many requests to fix this because people dont understand what is the root cause.

@rchiodo
Copy link
Contributor

rchiodo commented Jan 26, 2022

I did some more looking.

This line here is eliminating your global kernel spec:

(usingNonIpyKernelLauncher(item) || Object.keys(item.kernelSpec.env || {}).length > 0)

So it's not even considered (it matches an interpreter).

Then this line here generates a kernel name:

const spec = createInterpreterKernelSpec(i, rootSpecPath);

I think I have a fix for it. Maybe. I keep getting dupes in the kernel picker. Not sure how yet.

@rchiodo
Copy link
Contributor

rchiodo commented Jan 26, 2022

Ah figured out why there's a dupe. This is more complicated than thought. We have restrictions on interpreters have to load super fast (especially the current one) and there's no way to get the matching kernelspec at that point.

So it creates one based on the 'active' interpreter, but then it also creates one later that is slightly different (because it has the kernelspec on disk)

@rchiodo
Copy link
Contributor

rchiodo commented Jan 26, 2022

Here's the partial working branch.
https://github.com/microsoft/vscode-jupyter/tree/rchiodo/use_kernel_name

I think we could either:

  • Not preemptively generate a kernelspec from the active interpreter
  • Update the controller after the new list comes back

@rchiodo
Copy link
Contributor

rchiodo commented Jan 26, 2022

Another alternative, just let these kernelspecs through even if they don't have a custom environment.

@DonJayamanne
Copy link
Contributor

just let these kernelspecs through even if they don't have a custom environment.

agreed. do this for all non default kernelspecs (excluse python etc and similar default kernelsspecs in python env directories)

this way all custom (user created) kernelspecs are displayed.

@DonJayamanne
Copy link
Contributor

i think we already hide default kernel specs , all wed need to do is remove the code that looks for duplicate kernel specs based on interpreter. which i believe is a simple block of code

@greazer
Copy link
Contributor

greazer commented Jan 27, 2022

If the user is using a named kernelspec for a notebook on the machine, then we shouldn't hide it or fail to write it to metadata.

@greazer greazer added this to the February 2022 milestone Jan 27, 2022
@DonJayamanne DonJayamanne self-assigned this Feb 13, 2022
@DonJayamanne
Copy link
Contributor

This should now be fixed in the pre-release version.
@pplonski Please could you switch to the pre-release version of the Jupyter extesnsion (open the Jupyter extension from the extensions panel and click the button Switch to pre-release version
Once done, you might have to reload VS Code and it should work as expected.

Closing as this has been resolved as part of this issue #8481

@pplonski If the new pre-release version doesn't fix this please do let us know and we'll be happy to reopen this issue.

@pplonski
Copy link
Author

@rchiodo @DonJayamanne @greazer thank you for looking into this.

I've installed pre-release version:
image

  1. The first use case. I've created a new file with the name test.ipynb. It is using the suggested kernel (in my case the suggested kernel is dashenv).

The end of test.ipynb file:

... the rest of the file ...
 "metadata": {
  "interpreter": {
   "hash": "5803cc62ceeeb9687df5d554a2392a6d093e68555e7672edae03c11d23d523ae"
  },
  "kernelspec": {
   "display_name": "Python 3.8.10 ('dashenv': venv)",
   "language": "python",
   "name": "python3"
  },
  "language_info": {
   "codemirror_mode": {
    "name": "ipython",
    "version": 3
   },
   "file_extension": ".py",
   "mimetype": "text/x-python",
   "name": "python",
   "nbconvert_exporter": "python",
   "pygments_lexer": "ipython3",
   "version": "3.8.10"
  },
  "orig_nbformat": 4
 },
 "nbformat": 4,
 "nbformat_minor": 2
}

The name in kernelspec should be dashenv - so it is not working.

  1. In the same file I changed the kernel to different one. I set kernel venv (for example) and when kernel is selected manually then it is working as expected. The end of the file test.ipynb:
... the rest of the file ...
 "metadata": {
  "interpreter": {
   "hash": "5803cc62ceeeb9687df5d554a2392a6d093e68555e7672edae03c11d23d523ae"
  },
  "kernelspec": {
   "display_name": "venv",
   "language": "python",
   "name": "venv"
  },
  "language_info": {
   "codemirror_mode": {
    "name": "ipython",
    "version": 3
   },
   "file_extension": ".py",
   "mimetype": "text/x-python",
   "name": "python",
   "nbconvert_exporter": "python",
   "pygments_lexer": "ipython3",
   "version": "3.8.10"
  },
  "orig_nbformat": 4
 },
 "nbformat": 4,
 "nbformat_minor": 2
}
  1. Then I selected the suggested kernel again (dashenv). The end of the file test.ipynb:
... the rest of the file ...
 "metadata": {
  "interpreter": {
   "hash": "5803cc62ceeeb9687df5d554a2392a6d093e68555e7672edae03c11d23d523ae"
  },
  "kernelspec": {
   "display_name": "venv",
   "language": "python",
   "name": "python3"
  },
  "language_info": {
   "codemirror_mode": {
    "name": "ipython",
    "version": 3
   },
   "file_extension": ".py",
   "mimetype": "text/x-python",
   "name": "python",
   "nbconvert_exporter": "python",
   "pygments_lexer": "ipython3",
   "version": "3.8.10"
  },
  "orig_nbformat": 4
 },
 "nbformat": 4,
 "nbformat_minor": 2
}

The display_name and name are wrong in the kernelspec.

To summarize, it is partially working (for case 2). For the suggested kernel, it is not working.

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Feb 14, 2022

@pplonski
Thanks for providing the information, I'm confused with the steps used to repro this issue, please could you provide the following information:

  • Lets start this test from scratch
  • Please reload VS Code
  • Create a new notebook file (ipynb with the contents you expect, e.g. ensure you have the desired kernelspec name and display name)
    • Please remove the hash information (this is added by VS Code, hence this must be removed) & the file name also changed
  • Please provide the ipynb file used for testing (before you edit it in vscode), i'd like to see what is inside the ipynb before it gets updated by VS Code
  • Please provide a screenshot of the notebook with the selected kernel displayed on the top right handside of the notebook
  • Run a cell, and you will get prompted to select a kernel (please provide a screenshot of this list)
  • Save the notebook, please upload the ipynb file (I'd like to see what it gets updated to)
  • Please upload the contents of the jupyter output panel (please provide all of the logs)

@DonJayamanne DonJayamanne reopened this Feb 14, 2022
@DonJayamanne DonJayamanne added info-needed Issue requires more information from poster and removed needs-triage labels Feb 14, 2022
@pplonski
Copy link
Author

@DonJayamanne sorry for the confusion. I've started VS Code after machine restart and looks like it is working!

vs-code-reproduce

Thank you very much! Closing the issue.

@DonJayamanne
Copy link
Contributor

No apologies necessary, glad that you were able to verify this and get back to us.
Thanks again for the confirmation.

@DonJayamanne DonJayamanne added verified Verification succeeded and removed info-needed Issue requires more information from poster labels Feb 15, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants