Unify node/web kernel and remove iw reference in kernel component #10853
Conversation
rebornix
force-pushed
the
rebornix/unify-kernel
branch
from
c89f1e3 to
a95286f
Compare
Codecov Report
@@ Coverage Diff @@
## main #10853 +/- ##
========================================
+ Coverage 52% 63% +10%
========================================
Files 486 483 -3
Lines 33762 33711 -51
Branches 5500 5501 +1
========================================
+ Hits 17757 21366 +3609
+ Misses 14284 10305 -3979
- Partials 1721 2040 +319
|
| import { IExtensionContext } from '../../platform/common/types'; | ||
|
|
||
| @injectable() | ||
| export class InteractiveWindowDebuggingStartupCodeProvider implements IStartupCodeProvider { |
There was a problem hiding this comment.
Seems like these could be the same. Just use the fs.readFile on the extension URI for both.
There was a problem hiding this comment.
From what I remember, this same code applies to desktop extension as well, when debugging using remote jupyter kernels. Hence we need this same code for non-web extension as well
There was a problem hiding this comment.
aaah, I can see that this is the same code in InteractiveWindowDebuggingStartupCodeProvider.node & .web
Yeah, would prefer if we could have the same class to do both, rather than having two seprate classes with the same logic, then the logic for debugging remote jupyter kernels is in one place, instead of being split across two files.
|
|
||
| async getCode(kernel: IKernel): Promise<string[]> { | ||
| if (!this.isWebExtension) { | ||
| const useNewDebugger = this.configService.getSettings(undefined).forceIPyKernelDebugger === true; |
There was a problem hiding this comment.
Now I merged the code for both Web and Node. I'm not sure though if the setting check on Node is still necessary but I kept it @rchiodo
There was a problem hiding this comment.
Yeah this is really confusing. Maybe a comment? The addRunCellHook is necessary when using ipykernel 6 with the old interactive window debugger.
The old interactive window debugger only supported local though.
| if ( | ||
| !( | ||
| isLocalConnection(kernel.kernelConnectionMetadata) && | ||
| !this.configService.getSettings(undefined).forceIPyKernelDebugger |
There was a problem hiding this comment.
is it necessary to pass undefined as a parameter?
| traceInfo('UpdateWorkingDirectoryAndPath in Kernel'); | ||
| return this.getChangeDirectoryCode(kernel, suggestedDir); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure how much it would make sense to merge the tow ways to resolve the suggested dir, but I feel that with a new local method the code can be much simpler to parse. Something like:
private async resolveDirectory(kernel: IKernel, path?: Uri): Promise<Uri | undefined> {
let dir: Uri | undefined;
if (!path) {
dir = await calculateWorkingDirectory(
this.configService,
this.workspaceService,
this.fs,
kernel.resourceUri
)
} else {
dir = Uri.file(
expandWorkingDir(
getFilePath(path),
kernel.resourceUri,
this.workspaceService,
this.configService.getSettings(kernel.resourceUri)
)
);
}
if (dir && (await this.fs.exists(dir))) {
return dir;
}
return;
}Would allow us to do something like:
if (
isLocalConnection(kernel.kernelConnectionMetadata) ||
isLocalHostConnection(kernel.kernelConnectionMetadata)
) {
let suggestedDir = await this.resolveDirectory(kernel);
if (kernel.resourceUri && (await this.fs.exists(kernel.resourceUri))) {
suggestedDir = await this.resolveDirectory(kernel, suggestedDir);
}
if (suggestedDir) {
traceInfo('UpdateWorkingDirectoryAndPath in Kernel');
return this.getChangeDirectoryCode(kernel, suggestedDir);
}
}There was a problem hiding this comment.
if this doesn't look valuable, feel free to resolve/ignore it!
There was a problem hiding this comment.
Thanks, I'll consider having a new PR taking this suggestion!
Attempt to unify
kernel.web/node.base. While trying to merge them, I found that the major difference between them is how we should generate start up code for interactive window debugging. It feels wrong that kernel needs to be aware of the logic of interactive window debugging so this PR tries to switch this to provider model.@DonJayamanne let's meet offline and discuss about this PR.
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).