- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.3k
 
Add h2oGPT Client #133
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
Add h2oGPT Client #133
Conversation
| 
           @this Looking great! Make sure you merge main into your PR as the langchain had breaking changes with 1 extra API item. Also, gradio team fixed the Chat part: gradio-app/gradio#4081 (comment) I'll finish this PR now: #117  | 
    
| 
           @this How's this PR going?  | 
    
          
 @pseudotensor I'm currently working on the   | 
    
        
          
                client/h2ogpt_client/core.py
              
                Outdated
          
        
      | INSTRUCT_VICUNA = "instruct_vicuna" | ||
| INSTRUCT_WITH_END = "instruct_with_end" | ||
| OPEN_ASSISTANT = "open_assistant" | ||
| PLAIN = "plain" | 
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.
We should avoid dup with rest of code as this is constantly extended. I moved to enum the original version. Is there some reason to stick to FOO_BAR = "foo_bar" format?
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 some reason to stick to FOO_BAR = "foo_bar" format?
@pseudotensor I simply capitalized the value to derive the name of the enum. Id these names are not appropriate, please suggest good names.
All capitalized names were used by following the Python convention https://docs.python.org/3.8/library/enum.html#creating-an-enum. I noticed that PromptType enum names are all simple. Shall we change that?
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.
Yes, let's change to whatever is normal convention. Requires going through and changing conditionals where PromptType used.
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'll create a separate PR for this.
| max_time: int = 180, | ||
| repetition_penalty: float = 1.07, | ||
| number_returns: int = 1, | ||
| system_pre_context: 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.
For the list of parameters, can you (when building the client) verify that the parameters match those passed to evaluate, so it is up to date?  i.e. using eval_func_param_names .  It won't break things when they are not up to date if you are using string version of API, but would be good when building client that everything is synced so everything possible can be done with this client.  Thanks!
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.
E.g. you can at build time check in python that set of inspection arguments for "create" is larger than set from eval_func_param_names .  I do stuff like that in h2oGPT.
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.
ya, we can do that. Shall we create a new PR for this? In order to make eval_func_param_names isolated (so that it can be used in the client) I had to make changes in places where eval_func_param_names is used. I fell like if I add those changes into this PR, it will clutter 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.
Let's merge! Thanks! Let's do more testing and refactors in another PR.
| 
           Non-manual test failures on this branch: Unrelated to this PR.  | 
    
Fixes #80
Usage