8000 [Worker support] Kill _unwrapped_remote · Issue #1384 · pyscript/pyscript · GitHub
[go: up one dir, main page]
Skip to content

[Worker support] Kill _unwrapped_remote #1384

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.

Sign up for GitHub

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

Jump to bottom
Closed
antocuni opened this issue Apr 12, 2023 · 2 comments · Fixed by #1490
Closed

[Worker support] Kill _unwrapped_remote #1384

antocuni opened this issue Apr 12, 2023 · 2 comments · Fixed by #1490

Comments

@antocuni
Copy link
Contributor

This is a follow-up of #1333 (once it's merged).

We should kill the last remaining usage of _unwrapped_remote, because it works only in the main thread case and cannot work in the worker case.

AFAIK this is the last place where we use it:

pyscript/pyscriptjs/src/main.ts

Lines 397 to 402 in ef0e838

// eventually replace with interpreter.pyimport(modulename);
const module = interpreter._unwrapped_remote.pyimport(modulename);
if (typeof (await module.plugin) !== 'undefined') {
const py_plugin = module.plugin as PythonPlugin;
py_plugin.init(this);
this.plugins.addPythonPlugin(py_plugin);

Killing it should also make it possible to simplify and cleanup some ugly code such as this:

pyscript/pyscriptjs/src/main.ts

Lines 220 to 227 in ef0e838

const cfg = this.config.interpreters[0];
let x;
if (this.config.execution_thread == 'worker') {
x = await this._startInterpreter_worker(cfg);
} else {
x = await this._startInterpreter_main(cfg);
}
const { remote_interpreter, wrapped_remote_interpreter } = x;

@antocuni antocuni mentioned this issue Apr 12, 2023
Closed
@antocuni
Copy link
Contributor Author

I investigated a bit what's needed to kill _unwrapped_remote. I tried to apply to patch and run test_plugins.py:

diff --git a/pyscriptjs/src/main.ts b/pyscriptjs/src/main.ts
index 86eaf9e..e3e8a7f 100644
--- a/pyscriptjs/src/main.ts
+++ b/pyscriptjs/src/main.ts
@@ -415,7 +415,8 @@ export class PyScriptApp {
         //       interpreter API level and allow each one to implement it in its own way
 
         // eventually replace with interpreter.pyimport(modulename);
-        const module = interpreter._unwrapped_remote.pyimport(modulename);
+        //const module = interpreter._unwrapped_remote.pyimport(modulename);
+        const module = await interpreter._remote.pyimport(modulename);
         if (typeof (await module.plugin) !== 'undefined') {
             const py_plugin = module.plugin as PythonPlugin;
             py_plugin.init(this);

With -m main, they all pass apart these two:

test_pyscript_exec_hooks[main-chromium] FAILED
test_pyrepl_exec_hooks[main-chromium] FAILED

This is the traceback which I see in the console:
image

The traceback is very obscure and I cannot even understand in which part of our code originates from. It seems an internal synclink problem, but I'm not really sure. @hoodmane do you have a clue?

@hoodmane
Copy link
Contributor

Yes. It comes from Pyodide's callKwargs. The last argument is supposed to be an argument but instead it's a synclink proxy. I think the easiest fix is to make an extra transfer handler for kwargs objects. Probably add a symbol to mark an object as a kwargs.

@madhur-tandon madhur-tandon mentioned this issue May 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

kill unwrapped_remote madhur-tandon/pyscript
2 participants
@antocuni @hoodmane
0