dalai icon indicating copy to clipboard operation
dalai copied to clipboard

Preserve newlines and other formatting

Open rany2 opened this issue 1 year ago • 17 comments

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

rany2 avatar Mar 14 '23 15:03 rany2

fixed in PR #39

bernatvadell avatar Mar 14 '23 18:03 bernatvadell

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

rany2 avatar Mar 14 '23 18:03 rany2

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

" I am not having problems with the escaped characters:

image

bernatvadell avatar Mar 14 '23 19:03 bernatvadell

@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 avatar Mar 14 '23 20:03 rany2

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 avatar Mar 14 '23 20:03 rany2

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

rany2 avatar Mar 14 '23 20:03 rany2

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

image

bernatvadell avatar Mar 14 '23 20:03 bernatvadell

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

rany2 avatar Mar 14 '23 20:03 rany2

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 avatar Mar 14 '23 20:03 bernatvadell

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

bernatvadell avatar Mar 14 '23 22:03 bernatvadell

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

rany2 avatar Mar 14 '23 22:03 rany2

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.

bernatvadell avatar Mar 14 '23 22:03 bernatvadell

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.

rany2 avatar Mar 14 '23 22:03 rany2

Atm we're forcing to use powershell or bash:

image

bernatvadell avatar Mar 14 '23 23:03 bernatvadell

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

rany2 avatar Mar 14 '23 23:03 rany2

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");

bernatvadell avatar Mar 14 '23 23:03 bernatvadell

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

rany2 avatar Mar 14 '23 23:03 rany2