-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Mostly revert #11038 but without breaking notebooks. #11050
Mostly revert #11038 but without breaking notebooks. #11050
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Codecov Report
@@ Coverage Diff @@
## master #11050 +/- ##
==========================================
- Coverage 61.26% 61.23% -0.03%
==========================================
Files 593 593
Lines 32480 32484 +4
Branches 4592 4592
==========================================
- Hits 19898 19893 -5
- Misses 11580 11587 +7
- Partials 1002 1004 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the daemon should not be using the isolated script. execModule on the daemon isnt actually executing a process. It's loading the module manually.
The smoke test has to pass before you can submit. It's indicating this still isn't working:
|
// tslint:disable-next-line:no-suspicious-comment | ||
// TODO(gh-11049) Run isolated (drop the "false" arg). | ||
const args = internalPython.execModule(moduleName, [], false); | ||
const msg = { args }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a redundant variable, why not const msg = { args: internalPython.exec....
const msg = { args: ['-m', moduleName] }; | ||
// tslint:disable-next-line:no-suspicious-comment | ||
// TODO(gh-11049) Run isolated (drop the "false" arg). | ||
const args = internalPython.execModule(moduleName, [], false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels the API is incorrectly named, this is generating arguments but method indicates it's exciting a module. This is not obvious unless I read the code in the function.
Codecov Report
@@ Coverage Diff @@
## main #11050 +/- ##
==========================================
- Coverage 61.26% 59.98% -1.29%
==========================================
Files 593 734 +141
Lines 32480 49412 +16932
Branches 4592 8106 +3514
==========================================
+ Hits 19898 29638 +9740
- Misses 11580 18199 +6619
- Partials 1002 1575 +573
Continue to review full report at Codecov.
|
This is unnecessary now that the DS code has been moved out. |
For #10681
This effectively makes the following change to bd942a0 (as opposed to what was done in #11038):
This makes sure all the extension code is using the centralized "internal" functions when running Python.