Skip to content

examples : switch input_noecho to input_echo to remove negation #979

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
May 2, 2023

Conversation

deadprogram
Copy link
Contributor

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!

Copy link
Contributor

@DannyDaemonic DannyDaemonic left a comment

Choose a reason for hiding this comment

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

This looks good. I had to manually merge the bool input_echo = true; line in because it is now preceded by bool is_antiprompt = false;.

Everything works the same and it makes the examples easier to read. No downside in that. I can't speak for the developers of the project, but I like this change.

It will need to be rebased before being committed but I approve.

Signed-off-by: deadprogram <ron@hybridgroup.com>
@deadprogram deadprogram force-pushed the examples-input-echo branch from 8a0b82a to 618fda5 Compare April 25, 2023 17:55
@deadprogram
Copy link
Contributor Author

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

Pinging maintainers... 😸

@DannyDaemonic
Copy link
Contributor

DannyDaemonic commented May 2, 2023

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.

@deadprogram
Copy link
Contributor Author

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

@deadprogram
Copy link
Contributor Author

Closing, will revisit developer experience improvements in future iterations.

@deadprogram deadprogram closed this May 2, 2023
@ggerganov ggerganov reopened this May 2, 2023
@@ -250,7 +250,7 @@ int main(int argc, char ** argv) {
}

bool is_antiprompt = false;
bool input_noecho = false;
bool input_echo = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool input_echo = true;
bool input_echo = true;

@ggerganov ggerganov merged commit e2cd506 into ggml-org:master May 2, 2023
@deadprogram
Copy link
Contributor Author

@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?

@ggerganov
Copy link
Member

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

DannyDaemonic pushed a commit to DannyDaemonic/llama.cpp that referenced this pull request May 2, 2023
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.

3 participants