use python api for checking ipykernel version and cache kernels #6607
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6607 +/- ##
======================================
- Coverage 69% 69% -1%
======================================
Files 410 411 +1
Lines 28282 28336 +54
Branches 4205 4212 +7
======================================
- Hits 19752 19643 -109
- Misses 6877 7035 +158
- Partials 1653 1658 +5
|
src/client/api/pythonApi.ts
Outdated
| ): Promise<ProductInstallStatus> { | ||
| try { | ||
| const api = await this.apiProvider.getApi(); | ||
| return await api.isProductVersionCompatible(product, semVerRequirement, resource); |
There was a problem hiding this comment.
You don't need the await
| return await api.isProductVersionCompatible(product, semVerRequirement, resource); | |
| return api.isProductVersionCompatible(product, semVerRequirement, resource); |
src/client/api/pythonApi.ts
Outdated
| try { | ||
| const api = await this.apiProvider.getApi(); | ||
| return await api.isProductVersionCompatible(product, semVerRequirement, resource); | ||
| } catch (ex) { |
There was a problem hiding this comment.
If we're not doing anything with the exception, then ther'es no need to capture this.
src/client/api/types.ts
Outdated
| isProductVersionCompatible( | ||
| product: Product, | ||
| semVerRequirement: string, | ||
| resource?: InterpreterUri |
There was a problem hiding this comment.
I would change this to PythonEnvironment instead of InterpreterUri, thats an old approach, and very vague that has caused a lot of issues, We should be more specific about the target interpreter.
| return mainVersionNumber >= 6; | ||
| const result = await this.pythonInstaller.isProductVersionCompatible( | ||
| Product.ipykernel, | ||
| '>=6.0.0', |
There was a problem hiding this comment.
This isn't a valid semver.
We should ideally just pass 6.0.0 and then on the other end use the necessary API to check the minimum version by prefixing wqith >= or the like.
There was a problem hiding this comment.
It is valid and it works.
There's an example here: https://docs.npmjs.com/cli/v6/using-npm/semver#ranges
| const mainVersionNumber = Number(versionSplit[0]); | ||
| return mainVersionNumber >= 6; | ||
| const installer = await this.serviceContainer.get<IPythonInstaller>(IPythonInstaller); | ||
| const result = await installer.isProductVersionCompatible(Product.ipykernel, '>=6.0.0', interpreter); |
There was a problem hiding this comment.
I would change this to 6.0.0 and change the code at the other end to add the >= else its incorrect to pass >=6.0.0, i.e. if you are saying that minimum is 6.0.0, then why do we need >=.
There was a problem hiding this comment.
The text >= is actually part of the code used to check for the version compatiblity. (its very specific to the tool & mechanism used to check the version).
There was a problem hiding this comment.
What other end? The python extension? That doesn't make sense, the function exposed asks for the semVerRequirement. It doesn't make sense to give it half the string in one place and the rest in another.
Passing 6.0.0 is wrong, that means 'be exactly 6.0.0', that's not what we want. From their documentation, the parameter should be >=6.0.0 which means, 6.0.0 or above.
There was a problem hiding this comment.
passing just 6.0.0 doesn't work, it returns 'needs upgrade'
There was a problem hiding this comment.
The parameter should be minimum version, and it should be passed as a semver value.
Again, the requirement is what is the minimum version, not, what is the query that needs to be run to check the minimum version.
There was a problem hiding this comment.
I.e. in Python I would change the following if (semver.satisfies(version, semVerRequirement)) { as follows:
if (semver.satisfies(version, ${>=${semVerRequirement})) {
and change the argument name semVerRequirement to be mimimumVersion: Semver (this way its not just any arbitrary string, but a proper version.
Else someone looking at this might pass 6.0.0 and assume that works. (that's exactly how i interpreted this).
There was a problem hiding this comment.
I understand your comment, but I don't think its confusing. The parameter clearly reads semVerRequirement and not semVer or minimumVersion.
Also, doing what you're asking means losing functionality of isProductVersionCompatible. It would only be able to check for a minimum version, and the time might come when we need to check a maximum version, an exact version, or a range. We can do all that with the function as it is right now.
For #6391
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).