Skip to content
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

Adding tgis params to caikit api #324

Merged
merged 18 commits into from
Apr 2, 2024
Merged

Conversation

waleedqk
Copy link

@waleedqk waleedqk commented Feb 15, 2024

Add support for ReturnOptions, GeneratedTokens and InputTokens

As part of caikit there are some parameters that are hard-coded to default values in code.
In the TGIS implementation, users can set these options and get the appropriate response.
The parameters in questions are part of the ResponseOptions proto found here.

This PR updates the data-model so that the return from inference server can be part of the result if the appropriate parameter is set in caikit FastAPI endpoint.

waleedqk added 11 commits February 15, 2024 10:43
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
    git push origin $BRANCH
e remote-tracking branch 'origin/main' into ResponseOptions
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
waleedqk added 2 commits March 26, 2024 09:13
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
@waleedqk waleedqk changed the title First Draft: Adding tgis params to caikit api Adding tgis params to caikit api Mar 26, 2024
Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! A few questions/request for more test assertions

token_logprobs: str
Whether or not to include logprob for each returned token.
Applicable only if generated_tokens == true and/or input_tokens == true
token_ranks: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the module runtime parameters these all appear to be bool not str? (Seems preserve_input_text might not have been accurate either, if that could also be updated here)

tests/fixtures/__init__.py Show resolved Hide resolved
Signed-off-by: waleedqk <waleedqk@ibm.com>
Whether or not to include logprob for each returned token.
Applicable only if generated_tokens == true and/or input_tokens == true
token_ranks: bool
Whether or not to include rank of each returned token.
Copy link
Collaborator

Choose a reason for hiding this comment

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

token_ranks is added but there is no output allowing for rank information to be exposed. Is this intentional?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I had some trouble initially with figuring out the output for the token_ranks parameter.
Since it wasnt to core ask, I exposed the parameter but have not adjusted the output for it in the data-model or guardrails

waleedqk added 3 commits April 2, 2024 12:06
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
Signed-off-by: waleedqk <waleedqk@ibm.com>
@waleedqk waleedqk requested a review from evaline-ju April 2, 2024 17:49
Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the update!

@evaline-ju evaline-ju merged commit 42c3075 into caikit:main Apr 2, 2024
5 checks passed
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.

None yet

2 participants