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

examples : switch input_noecho to input_echo to remove negation

Open deadprogram opened this issue 2 years ago • 1 comments

This PR modifies the example to change the boolean input_noecho to input_echo to remove negation.

I was reading thru the code, and thought it would probably make it easier for further refactoring later.

Thanks to everyone who has been committing to this project!

deadprogram avatar Apr 14 '23 19:04 deadprogram

Thanks for the review @DannyDaemonic I just replaced the previous commit with another one that does the same thing, but matches the latest master and also rebased.

deadprogram avatar Apr 25 '23 17:04 deadprogram

Pinging maintainers... :smile_cat:

deadprogram avatar May 02 '23 06:05 deadprogram

I'm in favor of this change but unfortunately improving main is low priority right now. I've got a couple pending patches for main stuck in limbo myself.

I think part of the issue is that those with permissions are so busy iterating over the LLM code that they don't have time to spend on patches that only improve something that's currently working. When I have a patch that fixes something that's broken or helps fix something that's causing problems, it goes through at reasonable speeds, but when my patch just adds a convenience or even fixes something that's being done wrong but isn't actively crashing (like this), it tends to slip through the cracks.

I assume when the ggml and llama stuff has slowed down some more, things like this can be addressed. I can't think of a reason they would prefer using input_noecho over input_echo (unless there's some precedent or coding pattern it follows that I don't know about), but some of these other patches I've seen seem excessive or just add extra stuff only the author would use. So I think another part of it is just the influx of pull requests and no one really wanting to be the bad guy, and that further lowers the priorities of these subset of pull requests.

They could add a maintainer whose focus is just on main and the other code outside of ggml.c and llama.cpp, but even that is something they'd have to evaluate and spend time discussing, which could take away from the momentum they've built with the evaluation and inference development. Don't get me wrong, I do think there's value in maining a more legible and feature complete main, but sometimes when you just don't have the time, lower priority things fall by the side.

DannyDaemonic avatar May 02 '23 07:05 DannyDaemonic

@DannyDaemonic perhaps this PR should be closed then, and revisit this topic at a future moment to reduce noise level?

deadprogram avatar May 02 '23 07:05 deadprogram

Closing, will revisit developer experience improvements in future iterations.

deadprogram avatar May 02 '23 07:05 deadprogram

@ggerganov thanks for merging.

What do you think about a PR that added a make task to use something like astyle to enforce a specific code formatting for the project?

deadprogram avatar May 02 '23 16:05 deadprogram

Automatic style won't work - I really like vertical aligning things and AFAIK no formatter supports this

ggerganov avatar May 02 '23 17:05 ggerganov