llama.cpp icon indicating copy to clipboard operation
llama.cpp copied to clipboard

Add `--simple-io option` for subprocesses and separate console to it's own header and cpp

Open DannyDaemonic opened this issue 2 years ago • 3 comments

--simple-io uses only basic io functions that work when main is run as a subprocess. This fixes issues with child processes such as those seen in #1547 and #1491. It also happens to be a work around for #1382 by allowing people to switch back to the basic IO for use with Windows Terminal history (that lets you press up and down to scroll through your history).

The number of lines changed is actually not as big as it looks, but the console stuff was half the size of the common.cpp file so I broke it out into its own cpp and header file.

DannyDaemonic avatar May 22 '23 06:05 DannyDaemonic

This would be very useful for my use-case of running in node. Is there an update on this?

spencekim avatar Jun 05 '23 23:06 spencekim

I had a big project come up and I've been pretty busy. I'll rebase this tomorrow though and see if anyone wants to review it.

It might be easier for people to swallow if I add in more features allowed by this approach. With all the attention the server example has been getting, some user interface tweaks here might be nice as well.

The other thing we need to think about is that none of the other examples use the console read functions so I wonder if I should move the console.h/cpp files into the main project directory.

DannyDaemonic avatar Jun 06 '23 10:06 DannyDaemonic

Thanks, looking forward to it! I saw this related issue, which is exactly my use case: https://github.com/ggerganov/llama.cpp/pull/1491

spencekim avatar Jun 06 '23 17:06 spencekim

Don't mean to be a bother, but is there a path to getting this merged into master?

spencekim avatar Jun 09 '23 19:06 spencekim

Seeing as how people are still hitting the old subprocess issue, I've rebased this patch. I've also got other UI improvements I'd love to put through. I think technically you're supposed to put each group of changes in separate PRs but seeing as how few people like to review this sort of admittedly boring change, perhaps I should just make one larger PR that includes all the stuff at once sort of how the large server rewrite was done?

The only other thing noteworthy in this particular patch is the namespace. I put console in it's own namespace since there are a growing number of global utf and console functions. This also moves the global struct to console.o instead of main.o -- since you only ever have one console, there's no point in making someone declare a status tracking structure. (Other console libraries such as ncurses do the same thing.)

Given it's been a little while, I'd like to briefly restate the goals in the direction of these changes: to simplify main by moving the implementation of console interactions out of main.cpp, and make user interactions more intuitive (so we can move away from the sole use of Ctrl-C for example). And all while adhering to the no-additional-libraries rule.

DannyDaemonic avatar Aug 03 '23 12:08 DannyDaemonic

Have to wait for this one sooo much time ... Thank you.

mirek190 avatar Aug 03 '23 12:08 mirek190

Can’t test this at the moment but I think it is OK to merge

ggerganov avatar Aug 04 '23 10:08 ggerganov

Thanks. If it helps, I'm pretty good about bug reports related to my code. Even if I'm working on a paid job.

DannyDaemonic avatar Aug 04 '23 15:08 DannyDaemonic

--simple-io - win 11 - powershell console is not responding. I can't enter any text - only cltl+c works.

mirek190 avatar Aug 04 '23 21:08 mirek190

Ok, I'm only able to recreate the issue on Windows 11 with Powershell. It does seem to work fine if you run it through the command prompt. This is a strange one.

DannyDaemonic avatar Aug 05 '23 00:08 DannyDaemonic

I vaguely remembered seeing this before, and then I remembered, it was with this project! I was having trouble with powershell before I ever touched the io code. I downloaded an old version of llama.cpp, from before I changed any console code at all, and it has the same problem with powershell.

I just think no one uses the wide character versions of these functions. Let me see if I can come up with something.

DannyDaemonic avatar Aug 05 '23 03:08 DannyDaemonic

@mirek190 This was a weird one. I realized if you open a new terminal and the very first thing you do is run main, simple io works! The problem is console modes are persistent in Windows 11 powershell. This was not the case in the old command prompt. Other programs can set this value and it will affect ours.

For now the work around is to make sure it's the first (and only program) you run when you open a windows powershell terminal. I'll have a patch in a few minutes.

DannyDaemonic avatar Aug 05 '23 04:08 DannyDaemonic

Fixed with #2521. I hope it won't take long to get through. Again, in the meantime, just run main as the first program in your powershell instance. If you run other commands or --simple-io stops working for other reasons, you can just start a new powershell instance.

DannyDaemonic avatar Aug 05 '23 04:08 DannyDaemonic

Is working now . Thanks

mirek190 avatar Aug 07 '23 08:08 mirek190