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

Main.exe no longer functions interactively in some scenarios #1547

Closed
MrJackSpade opened this issue May 21, 2023 · 8 comments
Closed

Main.exe no longer functions interactively in some scenarios #1547

MrJackSpade opened this issue May 21, 2023 · 8 comments

Comments

@MrJackSpade
Copy link

MrJackSpade commented May 21, 2023

Scenarios:

  1. Creating the process using PInvoke CreateProcess in C#.
  2. Executing while connected through SSH

Llama no longer starts in interactive mode when request, does not return control using Ctrl + C, and does not return control when a reverse prompt is encountered.

This was working properly on 5ea4339 and broken as of 07e9ace

Was this change intentional?

Is there a way to work around it?

I'm not seeing anything under the release notes that would indicate it was intended, but it appears to be broken in every version I've tested after 07e9ace

@MrJackSpade MrJackSpade changed the title Llama.exe no longer functions interactively when started using CreateProcess under Windows Llama.exe no longer functions interactively in some scenarios May 21, 2023
@MrJackSpade MrJackSpade changed the title Llama.exe no longer functions interactively in some scenarios Main.exe no longer functions interactively in some scenarios May 21, 2023
@MrJackSpade
Copy link
Author

I know almost nothing about c++ but I took a wild stab in the dark and tried reverting "Fix for mingw (#1462)" locally, and it resolved the problem.

@DannyDaemonic
Copy link
Contributor

I'm still able to use it through SSH, could you provide more clarity on that? Do you have to do both CreateProcess through C# and then call it through SSH?

As a side note, the reverse prompt change may be due to #1032. You have to specify -i if you're using reverse prompts.

The release you state where things stopped working (07e9ace) was some time after #1462. When testing #1462, did you jump back to that point in git or did you just reverse the patch?

I do see how the patch might be incompatible with certain input streams. Someone else had an issue with an adjacent IO issue. I may make a --simple-io that is designed to work with programs that read and write directly to the process.

@MrJackSpade
Copy link
Author

Apologies. It looks like some information was accidentally removed and some not properly updated while I was attempting to diagnose the issue.

I'm still able to use it through SSH, could you provide more clarity on that?

I've only tried OpenSSH server on windows, using open SSH as a client. I encounter the bug while running it on a Windows Host connected by SSH, from a localhost Windows client. I found this after attempting to find a way to circumvent the issue which I originally found using CreateProcess. So its Either/Or, both are not required. I assuming (especially given the cause) that it has something to do with redirecting stdin, which happens when I'm calling CreateProcess and would also (I assume) happen when executing a process under SSH since the input needs to be piped over to the process.

As a side note, the reverse prompt change may be due to #1032. You have to specify -i if you're using reverse prompts.

I have both "-i" and "--interactive-first" specified. I'm not sure why #1462 would have affected these specifically but the failure to return control does disappear when #1462 is reverted.

The release you state where things stopped working (07e9ace) was some time after #1462. When testing #1462, did you jump back to that point in git or did you just reverse the patch?

When I drafted the ticket I was having difficulty testing back past that revision initially due to the model changes, which is why that was as far as I was able to trace it at the time. The models are large and take a while to download, so while I was waiting I decided to set up the project locally and try reverting that specific commit since I did notice it dealt with stdin, which seemed like a potential culprit.

When determining that the issue was #1462, I ended up reverting that specific commit against master, so I'm now locally running master-fab49c6 with the only local change being master-d2c59b8 (#1462) reverted. That felt like the most reliable way to ensure I had found the issue. With #1462 reverted locally, all issues disappear.

I do see how the patch might be incompatible with certain input streams. Someone else had an issue with an adjacent IO issue. I may make a --simple-io that is designed to work with programs that read and write directly to the process.

A --simple-io flag or something similar would be very helpful. I would submit a PR myself but I'm lucky I even managed to get it to build given my lack of experience with c++. Right now I'm keeping a local fork, but the huge advantage to being able to wrap the process is that I can extend the functionality of Llama.cpp and integrate it with applications, without needing to keep my own fork, or deal with merges/pushes/pulls/builds, and all the other headache. Just being able to grab the latest build and drop it into the bin folder of my API helps keep integration much simpler.

@DannyDaemonic
Copy link
Contributor

Yeah, I'm going to come up with a PR to address a couple issues. I have a bigger PR I want to make but I'd rather handle these issues sooner than later.

@Nukepayload2
Copy link

does not return control when a reverse prompt is encountered

I have the same problem. I'm trying to start main.exe with the Process.Start function of .NET 7. If I specify RedirectStandardInput = True in ProcessStartInfo, the main.exe will generate random questions and answers after the reverse prompt is encountered. The stderr of main.exe contains Reverse prompt: '### Human:' and == Running in interactive mode. ==. If I remove RedirectStandardInput = True, the main.exe works fine. But my program is unable to input prompts through stdin. This problem happens even I start main.exe with cmd /c.

@DannyDaemonic
Copy link
Contributor

Are you compiling it yourself? Are you able to try the --simple-io from #1558 to see if that works with .NET? If you don't know how (or prefer not) to checkout the branch, you can use this patch and apply it with patch -p1 < 1558.patch.

@Nukepayload2
Copy link

Are you able to try the --simple-io from #1558 to see if that works with .NET?

@DannyDaemonic
I've successfully built #1558 by myself with Visual Studio. The new --simple-io option solved my problem. It's compatible with the ProcessStartInfo.RedirectStandardInput property of .NET. Thanks for adding this flag.

@DannyDaemonic
Copy link
Contributor

Fixed in #1558.

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

No branches or pull requests

3 participants