Skip to content

Conversation

@bandoti
Copy link
Collaborator

@bandoti bandoti commented Oct 21, 2023

This change improves robustness of argument-parsing logic by allowing the caller to decide what to do when -h/--help is supplied, or when an invalid argument is supplied by the user.

@cebtenzzre
Copy link
Collaborator

Is the only functional change in this 100-line PR the fact that log_print_usage is now also called on errors? What would the caller want to do besides end the program if invalid command-line arguments were passed?

@bandoti
Copy link
Collaborator Author

bandoti commented Oct 21, 2023

Is the only functional change in this 100-line PR the fact that log_print_usage is now also called on errors? What would the caller want to do besides end the program if invalid command-line arguments were passed?

Currently, applications in the examples directory simply print the usage message. However, when embedding the common logic as a C/C++ plugin for interpreted languages (which is something I'm working on), it is always desirable not to exit when the arguments are wrong or help is requested. The desired behavior in that case is to let the higher-level language determine if/when to exit the application.

Due to the complexity and sheer number of arguments which can be supplied to llama-based applications, it is desirable to have the common logic parse/process command-line arguments instead of having to duplicate the work.

@cebtenzzre
Copy link
Collaborator

Are you proposing that common.cpp should become a public API? As far as I know, it is only intended to be a place for code that is shared between the internal example programs.

@bandoti
Copy link
Collaborator Author

bandoti commented Oct 21, 2023

Are you proposing that common.cpp should become a public API? As far as I know, it is only intended to be a place for code that is shared between the internal example programs.

I think there is sufficient functionality within common to consider moving (at least some) to a public API. As is evident by the number of applications within examples which use the functionality, there is certainly a use-case. The need, in my opinion, is to provide a "gentler" API for folks (mostly everyone?) who don't want to reinvent the wheel.

While that is a lot broader in scope than the changes proposed in this PR—a more flexible way to respond to user input always helps toward that end. 🙂

@bandoti
Copy link
Collaborator Author

bandoti commented Oct 22, 2023

@cebtenzzre Per your feedback I reduced the scope of the change by hiding the implementation in a new gpt_params_parse_ex function. That way, the code change is reduced to only common.h/cpp. Anyone who needs more granular error-handling logic can simply use the new function directly.

Copy link
Member

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

The common code might become some sort of higher-level API in the future, but currently this is not the focus and there are not plans to do so. It's strongly recommended that 3rd party projects avoid using it or depending on it.

bandoti and others added 2 commits November 1, 2023 14:02
Co-authored-by: Georgi Gerganov <[email protected]>
Co-authored-by: Georgi Gerganov <[email protected]>
@bandoti
Copy link
Collaborator Author

bandoti commented Nov 1, 2023

The common code might become some sort of higher-level API in the future, but currently this is not the focus and there are not plans to do so. It's strongly recommended that 3rd party projects avoid using it or depending on it.

Understood—I will dream up another solution once the code velocity slows. Thank you for the quick turnaround on this PR in the meantime!

@ggerganov ggerganov merged commit 0e40806 into ggml-org:master Nov 1, 2023
@bandoti bandoti deleted the parse-args-error-handling branch November 1, 2023 17:48
ggerganov added a commit that referenced this pull request Nov 1, 2023
olexiyb pushed a commit to Sanctum-AI/llama.cpp that referenced this pull request Nov 23, 2023
* Allow caller to handle help/argument exceptions

* Prepend newline to usage output

* Add new gpt_params_parse_ex function to hide arg-parse impl

* Fix issue blocking success case

* exit instead of returning false

* Update common/common.h

Co-authored-by: Georgi Gerganov <[email protected]>

* Update common/common.cpp

Co-authored-by: Georgi Gerganov <[email protected]>

---------

Co-authored-by: Georgi Gerganov <[email protected]>
olexiyb pushed a commit to Sanctum-AI/llama.cpp that referenced this pull request Nov 23, 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