-
Notifications
You must be signed in to change notification settings - Fork 159
Conversation
Thanks @Kasimir123 for the PR. Great idea! |
stdin does work if you are just trying to do a basic chatbot in a terminal. The callback allows people to use the python wrapper to build out applications using the llma model. For example now you could have a webui that can interact with the bot, or you can automatically feed it certain data as well. |
src/main.cpp
Outdated
// input stream is bad or EOF received | ||
return 0; | ||
} | ||
line = grab_text_callback().cast<std::string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to let the callback signal EOF
then check it here so that the function can terminate gracefully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kasimir123 Yeah You are right.
I had a similar idea but was working with a different approach. I want to have more control over the inference so I decided to re-implement the logic in a similar way.
What @nuance1979 is asking for (and many other people) is difficult to do from the callback.
@Kasimir123 let me know if you have any idea on how to signal EOF
, otherwise I will merge the PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can think of two ways to signal EOF:
- Use a special string (e.g.,
"[EOF]"
) or character (e.g.,"\x03"
); - Let the callback return a
Tuple[string, bool]
where the boolean means EOF.
How do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, we could merge this PR first and continue this discussion in another PR. @Kasimir123 @abdeladim-s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kasimir123 is not replying, so I don't know if he is still working on it.
@nuance1979, I already implemented a different logic for interactive mode. So, this approach won't be necessary any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh great! Are you going to make a PR soon? Look forward to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Was a busy weekend. I will look into this tomorrow and see what I can come up with. Sounds like @abdeladim-s already has a different approach but I want to see if its possible just out of curiosity, will share what I find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nuance1979 @abdeladim-s I managed to add EOF file handling by adding a check to see if the callback result was of type string. This means that if you want end of file you can return None and it will correctly exit the program. To test this change change your callback to:
def grab_text_callback() -> str:
inpt = input()
if inpt == "END":
return None
return inpt + "\n"
src/main.cpp
Outdated
// input stream is bad or EOF received | ||
py::handle x = grab_text_callback(); | ||
|
||
if (!py::isinstance<py::str>(x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Does it mean that I can return anything other than a Python str
to signal EOF?
If that's the idea, I would suggest the following:
- first check if
x is None
as the EOF signal; (ref: https://pybind11.readthedocs.io/en/stable/reference.html?highlight=is%20None#_CPPv4NK10object_api7is_noneEv) - then check if
isinstance(x, str)
and give out a warning if not;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want it to warn and exit if the type is not string or simply warn and try again for input? I personally prefer the warn and exit method as the other way could lead to some weird loops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what string do we want the warning to be? Something like: "Input was not of type py:str"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want it to warn and exit if the type is not string or simply warn and try again for input? I personally prefer the warn and exit method as the other way could lead to some weird loops.
Well if it exits, it's not a warning anymore; it's a failure. I think failure here is a bit heavy-handed; warn and ignore is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with the above. To test you can use:
def grab_text_callback() -> str:
inpt = input()
if inpt == "END":
return None
if inpt == "1":
return 1
if inpt == "dict":
return {1: 'help'}
return inpt + "\n"
src/main.cpp
Outdated
line.pop_back(); // Remove the continue character | ||
else if (!py::isinstance<py::str>(x)) | ||
{ | ||
new_text_callback("Input was not of type py:str"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think it's a good idea to use new_text_callback
to print out warning. I'm suggesting fprintf(stderr,...)
to give out the warning in a "side channel", without affecting the "main channel" at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep that warning message or have a different message? just before I do another commit changing the way we print it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the convention in this file, I would suggest:
fprintf(stderr, "%s : input was not of type py::str. will ignore.\n", __func__);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, just pushed the latest change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Can you update the README file to let users know that they should return None
from grab_text_callback
to signal EOF? Also I trust you've already tested it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the README with an example of how to run interactive mode as well since I saw people asking about that in the issues and a quick start will help. And yes, I have been running this on my end and recompiling and testing with the code snippet above each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Kasimir123 and @nuance1979 for the contribution & sorry for this late reply.
I will merge it for now.
# To signal EOF, return None | ||
if inpt == "END": | ||
return None | ||
return inpt + "\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about return inpt + "\n"
instead of return inpt
? Because when I test it, only return inpt
works correctly. It makes sense because the original std::getline()
call also returns a line without the trailing \n
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nuance1979
I think you are right, I will fix it.
def new_text_callback(text: str): | ||
print(text, end="", flush=True) | ||
|
||
def grab_text_callback() -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def grab_text_callback() -> Optional[str]:
I added an additional Python callback that allows main.cpp to grab user input from the Python script. This allows users to run interactive mode as if they were running the standard llama.cpp file. For testing I used the bob prompt from llama's prompts, this can be run with the following Python file: