-
Notifications
You must be signed in to change notification settings - Fork 1.3k
This is a draft to show our remaining reasons for having a fork of RustPython #4878
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
base: main
Are you sure you want to change the base?
Conversation
@@ -1,3 +1,3 @@ | |||
#![allow(clippy::all)] | |||
#![allow(unused)] | |||
include!(concat!(env!("OUT_DIR"), "/python.rs")); | |||
include!("../python.rs"); |
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.
could you give more context behind this?
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.
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.
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.
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.
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.
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, |
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.
ditto here. What do you use from vm
that requires it be public? Can we add an additional method/function to cover the usecase?
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.
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?
vm/src/stdlib/time.rs
Outdated
#[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"} | ||
} |
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.
This should be
#[cfg(all(target_arch = "wasm32", not(target_os = "wasi"), feature = "wasmbind"))]
fn _time(_vm: &VirtualMachine) -> PyResult<f64> {
// ..
}
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.
Addressed in #4875
vm/src/vm/vm_object.rs
Outdated
#[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) | ||
} |
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.
This should be
#[cfg(all(target_arch = "wasm32", not(target_os = "wasi"), feature = "wasmbind"))]
{
// ...
}
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.
Addressed in #4875
No we are not running in an interactive shell |
b64a9f9
to
b7b0a49
Compare
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.