Support relative import by palaviv · Pull Request #1050 · RustPython/RustPython

palaviv · 2019-06-21T08:55:16Z

Should solve #55.

coolreader18 · 2019-06-21T23:00:16Z

I'm not sure why WASM's not passing; I restarted the build and im hoping it was just a fluke.

palaviv · 2019-06-22T06:56:51Z

When I run the test locally it passes.

palaviv · 2019-06-22T20:08:26Z

I'm not sure why WASM's not passing; I restarted the build and im hoping it was just a fluke.

It is not a fluke for sure...
I am starting to debug the test to see the problem. I think we should find a better test for the wasm then the selenium.

windelbouwman · 2019-06-23T09:33:14Z

            .map(|symbol| vm.ctx.new_str(symbol.to_string()))
            .collect();
-        let module = vm.import(module, &vm.ctx.new_tuple(from_list))?;
+        let level = module.chars().take_while(|char| *char == '.').count();


Instead of splitting of the level here, it's better to modify the parser to be able to handle this such that have AST nodes with a level inside. This way, the level is already known at parse time, and is passed in as a second argument. See also: https://greentreesnakes.readthedocs.io/en/latest/nodes.html#imports

This might be a big change tough, so it can be done in a follow up pull request.

Sure. I will do that in a different PR.

palaviv · 2019-06-23T18:48:20Z

I am not very familiar with javascript... The only thing I get in the error message is [object Opaque].
I do not actually import anything in my test and especially nothing relative...
If anyone has any idea why is this failing or what I can do to understand why I would appreciate any help 🆘

windelbouwman · 2019-06-23T18:54:48Z

Debugging travis can be a pain. Is the issue not present on the master branch?

windelbouwman · 2019-06-23T18:57:09Z

Did you try to reproduce the scenario locally?

coolreader18 · 2019-06-23T18:57:56Z

Try rebasing to master, #1039 added an import function specifically for wasm that might fix the issue.

coolreader18 · 2019-06-23T18:58:35Z

@windelbouwman #1050 (comment)

palaviv · 2019-06-23T19:12:11Z

Did you try to reproduce the scenario locally?

It passes and everything look good on the demo on locally.

Try rebasing to master, #1039 added an import function specifically for wasm that might fix the issue.

Already did... I hoped it will be resolved by that as well.

I will try removing stuff from the change to pinpoint the problem with the WASM.

palaviv · 2019-06-23T19:53:26Z

I will try removing stuff from the change to pinpoint the problem with the WASM.

The problematic line is the copy of the local variables as parameter of __import__. My guess is that I am doing something wrong there but I am not sure what... Could one of you have a look @windelbouwman @coolreader18?

coolreader18 · 2019-06-23T21:22:58Z

That's so strange, because as far as I can tell based on the tests that import function isn't being called at all.

coolreader18 · 2019-06-23T21:23:42Z

                    func,
                    vec![
                        self.ctx.new_str(module.to_string()),
-                        self.get_locals().clone().as_object().clone(),


Once you do re-add this, it can just be self.get_locals().into_object()

coolreader18 · 2019-06-25T06:26:12Z

Aha! In the demo page, you can look at the global variable __RUSTPYTHON_ERROR_STACK and see (a slice):

core::panicking::panic_fmt::h0d6d5c8b201e3246@http://localhost:5000/2884fcd643f7322b02df.module.wasm:wasm-function[36148]:0xf3aeae
core::option::expect_failed::hae9ebb676e842a6f@http://localhost:5000/2884fcd643f7322b02df.module.wasm:wasm-function[29113]:0xeb4d33
core::option::Option<T>::expect::he72bf3761fad8028@http://localhost:5000/2884fcd643f7322b02df.module.wasm:wasm-function[11921]:0xc26318
rustpython_vm::vm::VirtualMachine::current_scope::h49f16e5d2524e3d7@http://localhost:5000/2884fcd643f7322b02df.module.wasm:wasm-function[16313]:0xd2cfa1
rustpython_vm::vm::VirtualMachine::get_locals::hbe0204e1c9c7fbdf@http://localhost:5000/2884fcd643f7322b02df.module.wasm:wasm-function[13198]:0xc7f2d4
rustpython_vm::vm::VirtualMachine::import::h1dd82715101d7e1b@http://localhost:5000/2884fcd643f7322b02df.module.wasm:wasm-function[1520]:0x5f1021
rustpython_vm::vm::VirtualMachine::try_class::hbeb6ef59ad4b76fe@http://localhost:5000/2884fcd643f7322b02df.module.wasm:wasm-function[2274]:0x6f4c29

Which is from here in vm.rs:

pub fn current_scope(&self) -> Ref<Scope> {
    let frame = self
        .current_frame()
        .expect("called current_scope but no frames on the stack");
    Ref::map(frame, |f| &f.scope)
}

Not sure exactly what to do about this, but that's the root of the problem. Odd. Does get_locals() pop a frame off the stack or something?

palaviv · 2019-06-25T14:49:45Z

Thanks @coolreader18. Are you seeing this when running locally or on travis? How can I get __RUSTPYTHON_ERROR_STACK?
Maybe this happens because StoredVirtualMachine has a different Scope?

palaviv · 2019-06-25T14:52:31Z

@coolreader18 can you share the full stacktrace please?

palaviv · 2019-06-25T15:11:03Z

My guess is this:
We are failing in the following cal to pyresult_to_jsresult in WASMVirtualMachine.run:

let result = vm.run_code_obj(code, scope.borrow().clone());
convert::pyresult_to_jsresult(vm, result)

When we run the code in the line before we acutally push a frame to the vm and then we pop it when we finish (vm.run_frame). So when we call pyresult_to_jsresult our vm does not have any frames. I will change the code to pass locals only if we have frames and we will see if that works.

coolreader18 · 2019-06-25T16:24:16Z

Hallelujah! Do you want to merge this now?

palaviv · 2019-06-25T20:26:16Z

Hallelujah! Do you want to merge this now?

Very very very much 😃

windelbouwman · 2019-06-26T08:28:18Z

Can we improve on error reporting so that we might be able to pinpoint these issues easier in the future?

coolreader18 · 2019-06-26T14:21:08Z

That's probably doable, I'll look into it.

palaviv closed this Jun 21, 2019

palaviv reopened this Jun 21, 2019

palaviv mentioned this pull request Jun 21, 2019

Implement import by using importlib #958

Closed

8 of 11 tasks

palaviv force-pushed the relative-import branch from 83414d1 to 5567566 Compare June 22, 2019 19:35

windelbouwman reviewed Jun 23, 2019

View reviewed changes

palaviv added 3 commits June 23, 2019 18:21

Support relative import

a5276df

Test relative import

ae52596

Test relative import from upper level

e8c0644

palaviv force-pushed the relative-import branch from bf574c5 to cf2d42c Compare June 23, 2019 15:21

coolreader18 reviewed Jun 23, 2019

View reviewed changes

palaviv force-pushed the relative-import branch from f41e6ba to 6c21dec Compare June 24, 2019 15:10

pass local variables to __import__ only if there is a frame

da4c0bc

palaviv force-pushed the relative-import branch from f9124b6 to da4c0bc Compare June 25, 2019 15:15

coolreader18 merged commit c96680a into RustPython:master Jun 25, 2019

palaviv mentioned this pull request Jun 29, 2019

Calculate import level at parsing #1083

Merged

Conversation

palaviv commented Jun 21, 2019

Uh oh!

coolreader18 commented Jun 21, 2019

Uh oh!

palaviv commented Jun 22, 2019

Uh oh!

palaviv commented Jun 22, 2019

Uh oh!

windelbouwman Jun 23, 2019

Choose a reason for hiding this comment

Uh oh!

windelbouwman Jun 23, 2019

Choose a reason for hiding this comment

Uh oh!

palaviv Jun 23, 2019

Choose a reason for hiding this comment

Uh oh!

palaviv commented Jun 23, 2019

Uh oh!

windelbouwman commented Jun 23, 2019

Uh oh!

windelbouwman commented Jun 23, 2019

Uh oh!

coolreader18 commented Jun 23, 2019

Uh oh!

coolreader18 commented Jun 23, 2019

Uh oh!

palaviv commented Jun 23, 2019

Uh oh!

palaviv commented Jun 23, 2019

Uh oh!

coolreader18 commented Jun 23, 2019

Uh oh!

coolreader18 Jun 23, 2019

Choose a reason for hiding this comment

Uh oh!

coolreader18 commented Jun 25, 2019

Uh oh!

palaviv commented Jun 25, 2019

Uh oh!

palaviv commented Jun 25, 2019

Uh oh!

palaviv commented Jun 25, 2019

Uh oh!

coolreader18 commented Jun 25, 2019

Uh oh!

palaviv commented Jun 25, 2019

Uh oh!

windelbouwman commented Jun 26, 2019

Uh oh!

coolreader18 commented Jun 26, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants