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

Preserve newlines and other formatting #43

Open
rany2 opened this issue Mar 14, 2023 · 17 comments
Open

Preserve newlines and other formatting #43

rany2 opened this issue Mar 14, 2023 · 17 comments

Comments

@rany2
Copy link

rany2 commented Mar 14, 2023

When using dalai, it strips newlines among other things. I believe this is so that it works in shell (not that you can't pass arguments with newlines, just needs quoting).

I propose the following:

  • store the prompt in a file (or use pipes for Unix) and use llama.cpp's -f instead of -p

This has the advantage of not needing to worry about escaping/sanitizing user input and will fix other issues I've observed, like:

  • it not being able to parse prompts with `
  • prompts with $, ect
@bernatvadell
Copy link

fixed in PR #39

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

I don't see anywhere in your change where you escape shell input. I'd much prefer if the prompt was in a file instead

@bernatvadell
Copy link

after the change in textarea and the responses encapsulate them in "

" I am not having problems with the escaped characters:

image

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

@bernatvadell There is no way your change made a difference. Try with a single-quote to get it to obviously not work. If you end up closing the quote it does work, sure but try to put anything special like $, `, ", <, etc... it does NOT work and gets passed as is to bash.

image

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

If you need a description, $(ls) ended up just running and the output of ls got sent to the model. This is not what should happen under any circumstances...

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

IMHO the most proper way to get around this is to put the prompt in some temp file and have the model read from it (using LLAMA's -f). I recommend against trying to escape because there will always be edge cases you will miss and it might be a hit or miss across different shells, OSes, etc... putting it in a file is a universal fix so to speak

@bernatvadell
Copy link

I think we should use the "args" to be able to correctly escape the input.

image

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

@bernatvadell Yes that would be ideal, no idea why it is spawning in a shell...

@bernatvadell
Copy link

I imagine it would be possible to import the llama.cpp project as a library, I haven't looked into it, and right now, as far as I know, it's published as a console program.

@bernatvadell
Copy link

Finally, I have needed to escape the characters, the ones that I have verified that give problems are these:
image

This seems to work correctly:
image

The change is in the TypeScript PR #44

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

Why do you escape when you could just call exec directly? You don't need to exec it in a shell ..

@bernatvadell
Copy link

Why do you escape when you could just call exec directly? You don't need to exec it in a shell ..

Originally it was like this, I'm going to try what you say.

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

Anyway this proposal makes me a bit uneasy because in different shells you need to escape different things. For example if user was using fish, then (ls) would execute also. Best solution IMO is to just put the prompt in a file or use whatever the programming language offers to execute a program without relying on a shell (while allowing you to define arguments as an array of strings of course)

Edit: also if this was a Windows user then none of your escaping would be relevant.

@bernatvadell
Copy link

Atm we're forcing to use powershell or bash:

image

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

Is executing it in a shell still needed? I think you could call node's spawn directly now.

@bernatvadell
Copy link

bernatvadell commented Mar 14, 2023

it works quite well.

image

Now directly spawns the process, the arguments do not need to be sanitized manually.

the prompt is stored in a temporary file.

image

You can change default directory to store temporal prompts:

   this.tempPromptsPath =
      process.env.TEMP_PROMPTS_PATH ?? path.resolve(os.tmpdir(), "prompt-tmp");

@rany2
Copy link
Author

rany2 commented Mar 14, 2023

Fantastic, thank you. That's a fast pace!

mirroredkube pushed a commit to mirroredkube/dalai that referenced this issue Mar 26, 2023
* Use buffering

* Use vector

* Minor

---------

Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
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

2 participants