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

Cannot use two irust_repls at the same time. #114

Closed
baiguoname opened this issue Apr 18, 2023 · 27 comments · Fixed by #115
Closed

Cannot use two irust_repls at the same time. #114

baiguoname opened this issue Apr 18, 2023 · 27 comments · Fixed by #115

Comments

@baiguoname
Copy link

baiguoname commented Apr 18, 2023

I have two object irust_repl: irust_repl1 and irust_repl2. When I call irust_repl1.eval(some_code) and irust_repl2.eval(some_code) at the same time, the latter has a link.exe error. I thought it maybe because that no matter how many irust_repl I have, they all is based on the same temp file, and so manupulating the same main.rs at the same time.
So, if I need run the irust_repls at the same time, what should I do?

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 18, 2023

Yes your guess is right it uses a hardcoded cargo project path, I can look later at making this configurable

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 18, 2023

I can't reproduce the issue on linux , theoretically it shouldn't error , because the state is kept in memory and each time eval runs it will copy that state from memory to disk

The only problem I can see is that if the code runs in multiple threads at the same time, there is a risk of things getting messed up (like one repl accessing another repl values)

I think I should fix this issue by giving each repl its unique project, but I'm not 100 % sure it will fix your issue, if you have a minimal code that code can reproduce the issue that would be great

This is what I'm using to test the multi thread issue for example

#[test]
fn two_repls_at_the_same_time() {
    let mut repl1 = Repl::default();
    let mut repl2 = Repl::default();
    repl1.insert("let a = 4;");
    repl2.insert("let a = 5;");

    let a1_thread =
        std::thread::spawn(move || repl1.eval("a").unwrap().output.parse::<u8>().unwrap());
    let a2_thread =
        std::thread::spawn(move || repl2.eval("a").unwrap().output.parse::<u8>().unwrap());

    assert_eq!(a1_thread.join().unwrap() + a2_thread.join().unwrap(), 9) // this might fail it might equal to 10
}

@baiguoname
Copy link
Author

I just test the code you provided above in Windows, it also works well.
The original error I produced is run on web page, it may be too verbose to paste here. But I reproduce the same error in irust repl terminal, like this:
图片
Firstly, I open two terminals, I input the following codes into both of them:
{ use std::{thread::sleep, time::Duration}; let a = 1; sleep(Duration::from_secs(10)); a }
And then , I run these two terminals at the same time. As can be seen in the picture, the second terminal raise the error.

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 18, 2023

Can you test this pr #115

@baiguoname
Copy link
Author

I just test the new irust_repl, it works well.
And what is amazing is that:

  1. A new irust_repl take short time to run for the first time;
  2. The directory temp/repls doesn't blow up as I create more and more irust_repl object.

I'm using a lot of IJulia, which is a repl for Julia. It is amazing but with a imperfection, by which the users suffer a lot: it takes too long for the first running. That problem also lies in evcxr , in a more severe way. I thought irust may solved this problem. Being grateful for your work, I thought in my personal view if irust have a full support for jupyter kernel, it will be a blazing edge for rust, and will contribute a lot to make rust more popular.

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 19, 2023

Nice, glad it worked, the repl structs currently doesn't clean the created paths , though I exposed already a function to do that and irust uses it, I think I'll just go ahead and make it automatic by implementing it on drop (so it works as you described)

I'll try to make some time and evaluate jupyter again, did you try https://github.com/sigmaSd/IRust/tree/master/crates/irust_repl#jupyter-kernel btw ? does it do what you expect ?
Its just a bare bone implementation, but it should do the basics.
I think jupyter support only make sense after fixing #104 , since after that adding jupyter should be straightforward

@baiguoname
Copy link
Author

baiguoname commented Apr 19, 2023

Yes, I tried https://github.com/sigmaSd/IRust/tree/master/crates/irust_repl#jupyter-kernel several times before on Windows, it didn't work. When I open jupyter and click the run button, it run down immediately and no output shows, the terminal also says the kernel is terminating. I guess there may be errors on the output part.

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 19, 2023

I tried the kernel in linux and it seems to work, did you follow the steps ? can you see the kernel with jupyter kernelspec list, also is the re executable in your path, can you test it to make sure (re 1+2 for example)

I'll have to find a windows machine to test otherwise

@baiguoname
Copy link
Author

I just tried again, re is executable in my path, and I run .\re 1+2 in the terminal can work well and irustkernel showed in the jupyter kernelspec list. But when I run the jupyter cell, the terminal has the same error. The following is the error:
Traceback (most recent call last): File "C:\ProgramData\Anaconda3\lib\site-packages\ipykernel\kernelbase.py", line 353, in dispatch_shell await result File "C:\ProgramData\Anaconda3\lib\site-packages\ipykernel\kernelbase.py", line 643, in execute_request reply_content = self.do_execute( File "D:\Rust\IRust-master\crates\irust_repl\irustkernel\irust.py", line 29, in do_execute output = self.eval(self.repl + code, self.get_deps()) File "D:\Rust\IRust-master\crates\irust_repl\irustkernel\irust.py", line 48, in eval output = subprocess.run(["re", deps, code], stdout=subprocess.PIPE) File "C:\ProgramData\Anaconda3\lib\subprocess.py", line 505, in run with Popen(*popenargs, **kwargs) as process: File "C:\ProgramData\Anaconda3\lib\subprocess.py", line 951, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "C:\ProgramData\Anaconda3\lib\subprocess.py", line 1420, in _execute_child hp, ht, pid, tid = _winapi.CreateProcess(executable, args), FileNotFoundError

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

That error mean that your path is not set up correctly, you can try changing 're' here https://github.com/sigmaSd/IRust/blob/master/crates/irust_repl/irustkernel/irust.py#L48 to the absolute path of the binary (C:://...path..here../re)

@baiguoname
Copy link
Author

I just tried that, and then there is a new type of error:
thread 'main' panicked at 'No code provided', crates\irust_repl\examples\re\main.rs:7:14 note: run withRUST_BACKTRACE=1environment variable to display a backtrace thread 'main' panicked at 'No code provided', crates\irust_repl\examples\re\main.rs:7:14 note: run withRUST_BACKTRACE=1environment variable to display a backtrace

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

Can you add a print(code) before this line https://github.com/sigmaSd/IRust/blob/master/crates/irust_repl/irustkernel/irust.py#L48

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

Also can we continue this conversation here instead #108

@baiguoname
Copy link
Author

I just tried it, it has the same error:
image

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

Maybe you need to run jupyter trust notebook-name.ipynb https://stackoverflow.com/questions/44943646/jupyter-notebook-not-trusted

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

also if that doesn't work I'll need a print here https://github.com/sigmaSd/IRust/blob/master/crates/irust_repl/irustkernel/irust.py#L20 , if there is no output that means jupyter is not sending any code to the kernel

@baiguoname
Copy link
Author

baiguoname commented Apr 20, 2023

First, I tried trust irust.ipynb, then run the cell, it has the same error;
Then, I add the print code to where as your point, it still has the same error:
image

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

I mean you should add print and see the console, its for debugging, I want to see if the code gets printed

@baiguoname
Copy link
Author

The console also didn't print the code. And I found this warning in the irust.py:
image

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

The warning is probably harmless since it does execute, it's weird that do_execute doesn't pass code, if you want to debug more you can try the echo kernel from here https://jupyter-protocol.readthedocs.io/en/latest/wrapperkernels.html

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

Btw for the path issue I remembered in windows you might need cmd prefix so instead of run(["re") you can try run(["cmd","/c", "re"

@baiguoname
Copy link
Author

I added the cmd prefix, it has the same error:
image

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

Ok so the cmd prefix does fix the first issue

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

you seem to have reamoved Code and deps variable, It should be run(cmd,/c,re,deps,code)

@baiguoname
Copy link
Author

Yes, after I add deps code, it can run fine now. But no output be printed:
image

@sigmaSd
Copy link
Owner

sigmaSd commented Apr 20, 2023

Yes thats probably normal, jupyer is not printing it , for debugging we should print to stderr instead so it shows up in the console

Can you try btw run("cmd","/c","re",deps,code) instead of the absolute path of re , does that still work ?

IF you have some feedback /suggestions / new issues on this kernel feel free to add it here #108

@baiguoname
Copy link
Author

Now everything works fine, and I put the comments to #108 .

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 a pull request may close this issue.

2 participants