Skip to content
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

This is a draft to show our remaining reasons for having a fork of RustPython #4878

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lastmjs
Copy link

@lastmjs lastmjs commented Apr 15, 2023

The wasm-bindgen stuff is being addressed here: #4875

Also in that PR we are talking about time, but the PR doesn't address a solution to that.

@@ -1,3 +1,3 @@
#![allow(clippy::all)]
#![allow(unused)]
include!(concat!(env!("OUT_DIR"), "/python.rs"));
include!("../python.rs");
Copy link
Member

Choose a reason for hiding this comment

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

could you give more context behind this?

Copy link
Author

Choose a reason for hiding this comment

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

In our environment which is a decentralized Wasm cloud environment, there are function complexity checks that are performed before a Wasm binary is allowed to be deployed. The complexity limit is currently 15_000, but the generated lolrpop python.rs file has a function called __reduce that is ~9000 lines long and has a complexity of 16_722. So for now we need to modify the generated python.rs file and split up the __reduce function. We have asked the protocol engineers of the Wasm environment to increase the complexity limit, but if you know of how to get lolrpop to generate less complex functions that would be great too.

Copy link
Member

Choose a reason for hiding this comment

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

but if you know of how to get lolrpop to generate less complex functions that would be great too.

Doesn't seem likely lalrpop/lalrpop#304. Hopefully at some point we should get a PEG parser which shouldn't result in such a behemoth of a function.

Copy link
Member

Choose a reason for hiding this comment

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

Idk if it is possible now, but if you can take input as *.pyc instead of *.py, skipping parsing from the embedded machine will be possible (maybe with little bit fix of rustphython).

@@ -22,7 +22,7 @@ use std::sync::atomic::Ordering;
/// });
/// ```
pub struct Interpreter {
vm: VirtualMachine,
pub vm: VirtualMachine,
Copy link
Member

Choose a reason for hiding this comment

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

ditto here. What do you use from vm that requires it be public? Can we add an additional method/function to cover the usecase?

Copy link
Contributor

@fanninpm fanninpm left a comment

Choose a reason for hiding this comment

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

Generally, it's better to make a function disappear than to crash the interpreter.

@lastmjs Does your use case involve running in an interactive shell?

Comment on lines 92 to 118
#[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))]
fn _time(_vm: &VirtualMachine) -> PyResult<f64> {
use wasm_bindgen::prelude::*;
#[wasm_bindgen]
extern "C" {
type Date;
#[wasm_bindgen(static_method_of = Date)]
fn now() -> f64;
#[cfg(feature = "wasmbind")]
{
use wasm_bindgen::prelude::*;
#[wasm_bindgen]
extern "C" {
type Date;
#[wasm_bindgen(static_method_of = Date)]
fn now() -> f64;
}
// Date.now returns unix time in milliseconds, we want it in seconds
return Ok(Date::now() / 1000.0);
}
// Date.now returns unix time in milliseconds, we want it in seconds
Ok(Date::now() / 1000.0)
panic!{"No date available"}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be

#[cfg(all(target_arch = "wasm32", not(target_os = "wasi"), feature = "wasmbind"))]
fn _time(_vm: &VirtualMachine) -> PyResult<f64> {
    // ..
}

Copy link

Choose a reason for hiding this comment

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

Addressed in #4875

Comment on lines 27 to 39
#[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))]
{
use wasm_bindgen::prelude::*;
#[wasm_bindgen]
extern "C" {
#[wasm_bindgen(js_namespace = console)]
fn error(s: &str);
#[cfg(feature = "wasmbind")]
{
use wasm_bindgen::prelude::*;
#[wasm_bindgen]
extern "C" {
#[wasm_bindgen(js_namespace = console)]
fn error(s: &str);
}
let mut s = String::new();
self.write_exception(&mut s, &exc).unwrap();
error(&s);
}
let mut s = String::new();
self.write_exception(&mut s, &exc).unwrap();
error(&s);
panic!("{}; exception backtrace above", msg)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be

#[cfg(all(target_arch = "wasm32", not(target_os = "wasi"), feature = "wasmbind"))]
{
    // ...
}

Copy link

Choose a reason for hiding this comment

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

Addressed in #4875

@lastmjs
Copy link
Author

lastmjs commented Apr 19, 2023

Generally, it's better to make a function disappear than to crash the interpreter.

@lastmjs Does your use case involve running in an interactive shell?

No we are not running in an interactive shell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants