8000 kill unwrapped_remote by madhur-tandon · Pull Request #1490 · pyscript/pyscript · GitHub
[go: up one dir, main page]
Skip to content

kill unwrapped_remote #1490

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

Merged
merged 4 commits into from
Jun 1, 2023
Merged

kill unwrapped_remote #1490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ee9fa8
kill unwrapped_remote
madhur-tandon May 27, 2023
fa7cedc
linting
madhur-tandon May 27, 2023
cf72e49
don't use callKwargs for python plugins
madhur-tandon May 27, 2023
5cde738
fix tests and improve types
madhur-tandon May 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions pyscriptjs/src/interpreter_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,17 @@ InterpreterClient class is responsible to request code execution
*/
export class InterpreterClient extends Object {
_remote: Synclink.Remote<RemoteInterpreter>;
_unwrapped_remote: RemoteInterpreter;
config: AppConfig;
/**
* global symbols table for the underlying interface.
* */
globals: Synclink.Remote<PyProxyDict>;
stdio: Stdio;

constructor(
config: AppConfig,
stdio: Stdio,
remote: Synclink.Remote<RemoteInterpreter>,
unwrapped_remote: RemoteInterpreter,
) {
constructor(config: AppConfig, stdio: Stdio, remote: Synclink.Remote<RemoteInterpreter>) {
super();
this.config = config;
this._remote = remote;
this._unwrapped_remote = unwrapped_remote;
this.stdio = stdio;
}

Expand Down
19 changes: 7 additions & 12 deletions pyscriptjs/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ export class PyScriptApp {
*/
const interpreterURL = interpreter_cfg.src;
await import(interpreterURL);
return { remote_interpreter, wrapped_remote_interpreter };
return wrapped_remote_interpreter;
}

async _startInterpreter_worker(interpreter_cfg: InterpreterConfig) {
Expand All @@ -222,8 +222,7 @@ export class PyScriptApp {
const worker = new Worker(base_url + '/interpreter_worker.js');
const worker_initialize: any = Synclink.wrap(worker);
const wrapped_remote_interpreter = await worker_initialize(interpreter_cfg);
const remote_interpreter = undefined; // this is _unwrapped_remote
return { remote_interpreter, wrapped_remote_interpreter };
return wrapped_remote_interpreter;
}

// lifecycle (4)
Expand All @@ -238,19 +237,17 @@ export class PyScriptApp {
}

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

this.interpreter = new InterpreterClient(
this.config,
this._stdioMultiplexer,
wrapped_remote_interpreter as Synclink.Remote<RemoteInterpreter>,
remote_interpreter,
);
await this.afterInterpreterLoad(this.interpreter);
}
Expand Down Expand Up @@ -413,11 +410,9 @@ export class PyScriptApp {
// TODO: This is very specific to Pyodide API and will not work for other interpreters,
// when we add support for other interpreters we will need to move this to the
// 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 = await interpreter.pyimport(modulename);
if (typeof (await module.plugin) !== 'undefined') {
const py_plugin = module.plugin as PythonPlugin;
const py_plugin = (await module.plugin) as PythonPlugin;
py_plugin.init(this);
this.plugins.addPythonPlugin(py_plugin);
} else {
Expand Down
74 changes: 62 additions & 12 deletions pyscriptjs/src/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ import { UserError, ErrorCode } from './exceptions';
import { getLogger } from './logger';
import { make_PyScript } from './components/pyscript';
import { InterpreterClient } from './interpreter_client';
import { make_PyRepl } from './components/pyrepl';

const logger = getLogger('plugin');
type PyScriptTag = InstanceType<ReturnType<typeof make_PyScript>>;
type PyReplTag = InstanceType<ReturnType<typeof make_PyRepl>>;

export class Plugin {
/** Validate the configuration of the plugin and handle default values.
Expand Down Expand Up @@ -84,7 +86,12 @@ export class Plugin {
* @param options.outEl The element that the result of the REPL evaluation will be output to.
* @param options.pyReplTag The <py-repl> HTML tag the originated the evaluation
*/
beforePyReplExec(options: { interpreter: InterpreterClient; src: string; outEl: HTMLElement; pyReplTag: any }) {
beforePyReplExec(options: {
interpreter: InterpreterClient;
src: string;
outEl: HTMLElement;
pyReplTag: PyReplTag;
}) {
/* empty */
}

Expand All @@ -100,7 +107,7 @@ export class Plugin {
interpreter: InterpreterClient;
src: string;
outEl: HTMLElement;
pyReplTag: HTMLElement;
pyReplTag: PyReplTag;
result: any;
}) {
/* empty */
Expand All @@ -126,10 +133,26 @@ export type PythonPlugin = {
configure?: (config: AppConfig) => Promise<void>;
afterSetup?: (interpreter: InterpreterClient) => Promise<void>;
afterStartup?: (interpreter: InterpreterClient) => Promise<void>;
beforePyScriptExec?: { callKwargs: (options: any) => Promise<void> };
afterPyScriptExec?: { callKwargs: (options: any) => Promise<void> };
beforePyReplExec?: { callKwargs: (options: any) => Promise<void> };
afterPyReplExec?: { callKwargs: (options: any) => Promise<void> };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed callKwargs to have actual list of args instead....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to make it so that we can use kwargs. I'll look into fixing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed callKwargs to have actual list of args instead....

good idea @madhur-tandon, it looks also cleaner and easier to read now

I would like to make it so that we can use kwargs. I'll look into fixing it.

yes, this looks like a good idea in general

beforePyScriptExec?: (interpreter: InterpreterClient, src: string, pyScriptTag: PyScriptTag) => Promise<void>;
afterPyScriptExec?: (
interpreter: InterpreterClient,
src: string,
pyScriptTag: PyScriptTag,
result: any,
) => Promise<void>;
beforePyReplExec?: (
interpreter: InterpreterClient,
src: string,
outEl: HTMLElement,
pyReplTag: PyReplTag,
) => Promise<void>;
afterPyReplExec?: (
interpreter: InterpreterClient,
src: string,
outEl: HTMLElement,
pyReplTag: PyReplTag,
result: any,
) => Promise<void>;
onUserError?: (error: UserError) => Promise<void>;
};

Expand Down Expand Up @@ -188,7 +211,9 @@ export class PluginManager {

async beforePyScriptExec(options: { interpreter: InterpreterClient; src: string; pyScriptTag: PyScriptTag }) {
await Promise.all(this._plugins.map(p => p.beforePyScriptExec?.(options)));
await Promise.all(this._pythonPlugins.map(p => p.beforePyScriptExec.callKwargs(options)));
await Promise.all(
this._pythonPlugins.map(p => p.beforePyScriptExec?.(options.interpreter, options.src, options.pyScriptTag)),
);
}

async afterPyScriptExec(options: {
Expand All @@ -198,22 +223,47 @@ export class PluginManager {
result: any;
}) {
await Promise.all(this._plugins.map(p => p.afterPyScriptExec?.(options)));
await Promise.all(this._pythonPlugins.map(p => p.afterPyScriptExec.callKwargs(options)));
await Promise.all(
this._pythonPlugins.map(
p => p.afterPyScriptExec?.(options.interpreter, options.src, options.pyScriptTag, options.result),
),
);
}

async beforePyReplExec(options: {
interpreter: InterpreterClient;
src: string;
outEl: HTMLElement;
pyReplTag: any;
pyReplTag: PyReplTag;
}) {
await Promise.all(this._plugins.map(p => p.beforePyReplExec?.(options)));
await Promise.all(this._pythonPlugins.map(p => p.beforePyReplExec.callKwargs(options)));
await Promise.all(
this._pythonPlugins.map(
p => p.beforePyReplExec?.(options.interpreter, options.src, options.outEl, options.pyReplTag),
),
);
}

async afterPyReplExec(options: { interpreter: InterpreterClient; src: string; outEl; pyReplTag; result }) {
async afterPyReplExec(options: {
interpreter: InterpreterClient;
src: string;
outEl: HTMLElement;
pyReplTag: PyReplTag;
result: any;
}) {
await Promise.all(this._plugins.map(p => p.afterPyReplExec?.(options)));
await Promise.all(this._pythonPlugins.map(p => p.afterPyReplExec.callKwargs(options)));
await Promise.all(
this._pythonPlugins.map(
p =>
p.afterPyReplExec?.(
options.interpreter,
options.src,
options.outEl,
options.pyReplTag,
options.result,
),
),
);
}

async onUserError(error: UserError) {
Expand Down
8 changes: 0 additions & 8 deletions pyscriptjs/tests/integration/test_plugins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,10 @@ class ExecTestLogger(Plugin):
async def beforePyScriptExec(self, interpreter, src, pyScriptTag):
console.log(f'beforePyScriptExec called')
console.log(f'before_src:{src}')
console.log(f'before_id:{pyScriptTag.id}')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyScriptTag is a pyodide.ffi.JsProxy and I was unable to get the id.

I have removed this for now...since it wasn't testing something of any significant value I guess...
But of course, if there's a way to resolve this, we should re-add this stuff...

Copy link
Contributor
@JeffersGlass JeffersGlass May 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyScriptTag should be unwrapped as an HtmlElement within Python, and not appear like a JsProxy in this context... I was having similar issues with the event listener work, with some methods/attributes on JsProxies not proxying properly.

We should try to understand why this regression happened, because it means something in the way the JS objects are proxied into Python is broken.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pyScriptTag should be unwrapped as an HtmlElement within Python, and not appear like a JsProxy in this context

I'm confused. pyScriptTag is a JS object of clas HTMLElement, isn't it? If so, then it must be a JSProxy, because that's the way in which pyodide passes JS objects into Python.
What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I phrased my response poorly. This is my concern:

pyScriptTag is a pyodide.ffi.JsProxy and I was unable to get the id. I have removed this for now...

Accessing the id attribute here previous worked, and now doesn't. Which means attribute access on JsProxies is broken, in some way we don't understand. Does it need an await? Is it specific to async contexts? We don't know, but this previously worked and now doesn't.

Specific to this code, does this mean users can't access attributes of the pyScriptTag at all, and if not, what's the value in passing it?


async def afterPyScriptExec(self, interpreter, src, pyScriptTag, result):
console.log(f'afterPyScriptExec called')
console.log(f'after_src:{src}')
console.log(f'after_id:{pyScriptTag.id}')
console.log(f'result:{result}')


Expand All @@ -88,12 +86,10 @@ class PyReplTestLogger(Plugin):
def beforePyReplExec(self, interpreter, src, outEl, pyReplTag):
console.log(f'beforePyReplExec called')
console.log(f'before_src:{src}')
console.log(f'before_id:{pyReplTag.id}')

def afterPyReplExec(self, interpreter, src, outEl, pyReplTag, result):
console.log(f'afterPyReplExec called')
console.log(f'after_src:{src}')
console.log(f'after_id:{pyReplTag.id}')
console.log(f'result:{result}')


Expand Down Expand Up @@ -261,9 +257,7 @@ def test_pyscript_exec_hooks(self):
# These could be made better with a utility function that found log lines
# that match a filter function, or start with something
assert "before_src:x=2; x" in log_lines
assert "before_id:pyid" in log_lines
assert "after_src:x=2; x" in log_lines
assert "after_id:pyid" in log_lines
assert "result:2" in log_lines

@skip_worker("FIXME: relative paths")
Expand All @@ -286,9 +280,7 @@ def test_pyrepl_exec_hooks(self):
# These could be made better with a utility function that found log lines
# that match a filter function, or start with something
assert "before_src:x=2; x" in log_lines
assert "before_id:pyid" in log_lines
assert "after_src:x=2; x" in log_lines
assert "after_id:pyid" in log_lines
assert "result:2" in log_lines

@skip_worker("FIXME: relative paths")
Expand Down
7 changes: 1 addition & 6 deletions pyscriptjs/tests/unit/pyodide.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ describe('RemoteInterpreter', () => {
config,
stdio,
wrapped_remote_interpreter as Synclink.Remote<RemoteInterpreter>,
remote_interpreter,
);

/**
Expand Down Expand Up @@ -69,10 +68,6 @@ describe('RemoteInterpreter', () => {
expect(interpreter).toBeInstanceOf(InterpreterClient);
});

it('should check if interpreter is an instance of RemoteInterpreter', async () => {
expect(interpreter._unwrapped_remote).toBeInstanceOf(RemoteInterpreter);
});

it('should check if interpreter can run python code asynchronously', async () => {
expect((await interpreter.run('2+3')).result).toBe(5);
});
Expand All @@ -85,7 +80,7 @@ describe('RemoteInterpreter', () => {

it('should check if interpreter is able to load a package', async () => {
stdio.reset();
await interpreter._unwrapped_remote.loadPackage('numpy');
await interpreter._remote.loadPackage('numpy');
await interpreter.run('import numpy as np');
await interpreter.run('x = np.ones((10,))');
await interpreter.run('print(x)');
Expand Down
0