Skip to content

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

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

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

DannyDaemonic
Copy link
Contributor

@DannyDaemonic DannyDaemonic commented May 22, 2023

--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.

Edit:
Confirmed fix for #1491.
Confirmed fix for #1547.
Confirmed fix for #2477.

@spencekim
Copy link

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

@DannyDaemonic
Copy link
Contributor Author

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.

@spencekim
Copy link

Thanks, looking forward to it! I saw this related issue, which is exactly my use case: #1491

Copy link

@mirek190 mirek190 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks legit

@spencekim
Copy link

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

@DannyDaemonic
Copy link
Contributor Author

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.

@mirek190
Copy link

mirek190 commented Aug 3, 2023

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

@ggerganov
Copy link
Member

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

@DannyDaemonic
Copy link
Contributor Author

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 DannyDaemonic merged commit 3498588 into ggml-org:master Aug 4, 2023
@mirek190
Copy link

mirek190 commented Aug 4, 2023

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

@DannyDaemonic
Copy link
Contributor Author

DannyDaemonic commented Aug 5, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

DannyDaemonic commented Aug 5, 2023

@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
Copy link
Contributor Author

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.

@mirek190
Copy link

mirek190 commented Aug 7, 2023

Is working now .
Thanks

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 this pull request may close these issues.

4 participants