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
Harrison/add huggingface hub #23
Conversation
repo_id = values.get("repo_id", DEFAULT_REPO_ID) | ||
values["client"] = InferenceApi( | ||
repo_id=repo_id, | ||
token=os.environ["HUGGINGFACEHUB_API_TOKEN"], |
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.
These parameters seem like nice ones to include as init args with optional env args as backups.
Also the relationship feels a bit convoluted at first glance
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.
what in particular seems convoluted?
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 think the main thing is that the client is created in a validation function - but that seems out of scope of this PR given the existing structure
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.
ah yeah i agree. is a bit weird. may look into post_init
if "error" in response: | ||
raise ValueError(f"Error raised by inference API: {response['error']}") | ||
text = response[0]["generated_text"][len(prompt) :] | ||
if stop is not None: |
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.
Could we share this across models?
Yeah it's a bit hacky but I don't see better support via InferenceAPI and ultimately it just ends with a bit of wasted computation
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.
what do you mean by share across models? its already factored out and used in cohere as well (cohere is a bit different - you can pass stop words but they are included at the end of the prompt)
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.
Yeah this is sufficient - not enough examples to merit more
from typing import List | ||
|
||
|
||
def enforce_stop_tokens(text: str, stop: List[str]) -> 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.
Double checking - the prompt isn't returned as part of the generated text, right?
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 it shouldnt be
…lback_docs Update examples to use inheritable callbacks
Add support for huggingface hub
I could not find a good way to enforce stop tokens over the huggingface hub api - that needs to hopefully be cleaned up in the future