return correct finish reasons in generate_text_func #210

dtrifiro · 2023-09-28T16:07:23Z

The finish reason in generate_text_func is currently broken (always returns OTHER). The reason is that a comparison is being made between a tensor element (int) and a string (eos_token) without converting it using the tokenizer

This PR fixes the tokenization issue and adds a few type hints

Full reproduction screiopt below

Reproduction script

#!/bin/sh
set -exu

tempdir="$(mktemp -d)"
cd ${tempdir}

python -m venv .venv 
source .venv/bin/activate
pip install git+https://github.com/caikit/caikit-nlp

grpcurl="${tempdir}/grpcurl"

# this requires git-lfs to be installed and `git lfs install` to have been run
if ! command -v git-lfs &>/dev/null; then echo "this requires git-lfs"; exit 1; fi

git clone https://huggingface.co/google/flan-t5-small

# convert the model for caikit
python -c 'import caikit_nlp; model = caikit_nlp.text_generation.TextGeneration.bootstrap("flan-t5-small"); model.save("flan-t5-small-caikit")'

CAIKIT_LOG_FILE=$(mktemp)

export TRANSFORMERS_CACHE="$PWD"
export RUNTIME_LIBRARY=caikit_nlp
export RUNTIME_LOCAL_MODELS_DIR="$PWD"

python -m caikit.runtime.grpc_server &>$CAIKIT_LOG_FILE &

# wait for caikit to properly start
timeout --signal=SIGINT 30 bash -c "grep -m1 'Caikit Runtime is serving on port' <(tail -f $CAIKIT_LOG_FILE)"

cat $CAIKIT_LOG_FILE

# install grpcurl to actually query the endpoint
curl -sL https://github.com/fullstorydev/grpcurl/releases/download/v1.8.7/grpcurl_1.8.7_linux_x86_64.tar.gz | tar zxvf - grpcurl
./grpcurl -plaintext -d '{"text": "At what temperature does liquid Nitrogen boil?"}' -H "mm-model-id: flan-t5-small-caikit" localhost:8085 caikit.runtime.Nlp.NlpService/TextGenerationTaskPredict
rc=$?

kill %1
exit $c

dtrifiro · 2023-09-28T16:08:12Z

Tagging @Xaenalt for visibility

caikit_nlp/toolkit/text_generation/model_run_utils.py

dtrifiro · 2023-09-28T16:15:31Z

A small sidenote here:

caikit-nlp/caikit_nlp/toolkit/text_generation/model_run_utils.py

Lines 236 to 241 in ef283e4

    
           if generate_ids[0][-1].item() == eos_token: 
        
               finish_reason = "EOS_TOKEN" 
        
           elif generate_ids.size(1) - 1 == max_new_tokens: 
        
               finish_reason = "MAX_TOKENS" 
        
           else: 
        
               finish_reason = "OTHER"

If finish_reason is set to OTHER, caikit currently fails to serialize the response because that's not defined in the FinishReason enum in caikit:
https://github.com/caikit/caikit/blob/4d01c9b4caded5492ca5772875f0d8778143a657/caikit/interfaces/nlp/data_model/text_generation.py#L35-L44

Would it make sense to add it there?

Xaenalt · 2023-09-28T21:06:25Z

A small sidenote here:

caikit-nlp/caikit_nlp/toolkit/text_generation/model_run_utils.py

Lines 236 to 241 in ef283e4

if generate_ids[0][-1].item() == eos_token:

finish_reason = "EOS_TOKEN"

elif generate_ids.size(1) - 1 == max_new_tokens:

finish_reason = "MAX_TOKENS"

else:

finish_reason = "OTHER"

If finish_reason is set to OTHER", caikitcurrently fails to serialize the response because that's not defined in theFinishReason` enum in caikit: https://github.com/caikit/caikit/blob/4d01c9b4caded5492ca5772875f0d8778143a657/caikit/interfaces/nlp/data_model/text_generation.py#L35-L44

Would it make sense to add it there?

I think it does make sense to add it there, especially if it's being referenced here. @gabe-l-hart is this intended?

gabe-l-hart · 2023-09-28T22:55:25Z

Hm, that does seem troubling. @gkumbhat I'll defer to you on this, but I think we probably do need to add OTHER to the enum in caikit/interfaces

gkumbhat

Just left a small comment, but otherwise looks good. Thanks for catching this and contributing the fix.

gkumbhat · 2023-09-29T02:51:33Z

caikit_nlp/toolkit/text_generation/model_run_utils.py

@@ -36,6 +36,11 @@
 # Local
 from ...data_model import ExponentialDecayLengthPenalty

+if TYPE_CHECKING:
+    # Third Party
+    from transformers import AutoModel, AutoTokenizer


Lets include this as a dependency since anyways all these modules anyways do need to work with transformers

Not sure what you mean here: get rid of if TYPE_CHECKING? transformers is already a dependency in pyproject.toml

@dtrifiro yep. thats exactly what I was suggesting

gkumbhat · 2023-09-29T04:22:38Z

Actually OTHER here is representing a lot of possible scenarios. We need to map it back to proper reasons I think. The enum we have in caikit is aligned with HF and with TGIS here. Adding OTHER in caikit interfaces would cause confusion and would not be informative. 🤔

dtrifiro · 2023-09-29T12:56:38Z

Thinking about it some more, the only other finish reason that should be returned by this function is STOP_SEQUENCE.
~~By quickly glancing at the code, I'm not sure this scenario works right now. I propose opening a new issue and tackle this in other PR.~~

I think the overall objective would be to get rid of OTHER here since it breaks the protobuf protocol anyway.

EDIT: I ended up adding some code to handle the STOP_SEQUENCE finish reason

caikit_nlp/toolkit/text_generation/model_run_utils.py

dtrifiro · 2023-10-07T13:08:11Z

@gkumbhat Another fix which was required in order for CI to pass with the new exception instead of OTHER as finish reason, is to fix the finish_reason logic for MAX_TOKENS, since it was currently broken causing most tests to break. See the last commit

gkumbhat · 2023-10-10T02:26:46Z

caikit_nlp/toolkit/text_generation/model_run_utils.py

@@ -237,7 +237,9 @@ def generate_text_func(
        generate_ids[0, -1] == tokenizer.eos_token_id
    ):
        finish_reason = "EOS_TOKEN"
-    elif generate_ids.size(1) - 1 == max_new_tokens:
+    elif (generate_ids.size(1) - 1 == max_new_tokens) or (
+        generate_ids.size(1) - inputs["input_ids"].size(1) == max_new_tokens


hmm. technically input will only be in output for causal-lm type of models, so there can be some side-effects in this calculations 🤔 May be we would need to check if input is actually in output and then calculate max_new_tokens for that accordingly

@gkumbhat I agree. I gave some thought to it and I ended up with the logic in the latest push. The idea is the following:

Check if the last token is eos_token, if so, finish_reason = "EOS_TOKEN"

Check if the stop sequence is the the generated tokens, if so, finish_reason = "STOP_SEQUENCE"

If none of the above conditions are true, then finish_reason = "MAX_TOKENS"

My reasoning for always returning MAX_TOKENS comes from looking at caikit.interfaces.nlp.data_model.text_generation.FinishReason. Here's the code with some comments I added

@dataobject(package=NLP_PACKAGE) class FinishReason(Enum): NOT_FINISHED = 0 # should only be returned by streaming implementations MAX_TOKENS = 1 EOS_TOKEN = 2 # matches rule #1 above CANCELLED = 3 # should not be set by generate_text_func TIME_LIMIT = 4 # should not be set by generate_text_func STOP_SEQUENCE = 5 # matches rule #2 above TOKEN_LIMIT = 6 # not sure about this ERROR = 7 # fairly generic,

I'm unsure about TOKEN_LIMIT (how is this different from MAX_TOKENS?), but I think none of the other enum values apply to generate_text_func .

TOKEN_LIMITrefers to the maximum number of tokens limit defined by the model whereas the MAX_TOKENS refers to the maximum number defined by the user. So one can reach TOKEN_LIMIT before MAX_TOKENS

@gkumbhat Ok, so that means that also need to check whether we reached the token limit for the model, although I'm not sure how we can get that information. Can we open an issue for adding support for TOKEN_LIMIT so that we can work on that on another PR?

Created #253

Signed-off-by: Daniele Trifirò <[email protected]>

…text_func "OTHER" is an invalid value for caikit.interfaces.nlp.data_model.text_generation.FinishReason, resulting in failed serialization of responses when querying the text generation endpoint. For `generate_text_func`, it is reasonable to assume that if the finish reason is not `EOS_TOKEN` or `STOP_SEQUENCE`, it must be `MAX_TOKENS`. Signed-off-by: Daniele Trifirò <[email protected]>

gkumbhat

LGTM

dtrifiro · 2023-10-31T12:13:59Z

Hey @gkumbhat, is there anything I can do to get this merged?

dtrifiro requested review from alex-jw-brooks, gkumbhat, evaline-ju, gabe-l-hart and tharapalanivel as code owners September 28, 2023 16:07

dtrifiro force-pushed the fix-finish-reason branch from cba775e to 7e31e6a Compare September 28, 2023 16:08

dtrifiro commented Sep 28, 2023

View reviewed changes

caikit_nlp/toolkit/text_generation/model_run_utils.py Show resolved Hide resolved

gkumbhat reviewed Sep 29, 2023

View reviewed changes

dtrifiro force-pushed the fix-finish-reason branch from bad7bc6 to 4d15fda Compare October 2, 2023 08:52

dtrifiro requested a review from gkumbhat October 3, 2023 15:23

gkumbhat reviewed Oct 3, 2023

View reviewed changes

caikit_nlp/toolkit/text_generation/model_run_utils.py Show resolved Hide resolved

gkumbhat reviewed Oct 10, 2023

View reviewed changes

dtrifiro mentioned this pull request Oct 11, 2023

[STORY] Split caikit/TGIS ServingRuntime into three distinct images opendatahub-io/caikit#11

Closed

dtrifiro added 3 commits October 27, 2023 10:54

model_run_utils: add/fix type hints

7fff5c0

Signed-off-by: Daniele Trifirò <[email protected]>

model_run_utils: fix logic for EOS_TOKEN finish reason

ac28f6a

Signed-off-by: Daniele Trifirò <[email protected]>

model_run_utils: add STOP_SEQUENCE finish reason

185ff22

Signed-off-by: Daniele Trifirò <[email protected]>

dtrifiro force-pushed the fix-finish-reason branch from c35fd62 to 86271dc Compare October 27, 2023 10:57

dtrifiro changed the title ~~return correct finish reason in generate_text_func~~ return correct finish reasons in generate_text_func Oct 27, 2023

dtrifiro force-pushed the fix-finish-reason branch from 86271dc to 1b45817 Compare October 27, 2023 11:23

dtrifiro mentioned this pull request Oct 27, 2023

fix uncaught exception in http client #247

Merged

gkumbhat approved these changes Oct 27, 2023

View reviewed changes

dtrifiro mentioned this pull request Oct 30, 2023

generate_text_func: support finish_reason=TOKEN_LIMIT #253

Open

gkumbhat merged commit c5ab581 into caikit:main Oct 31, 2023
4 checks passed

dtrifiro deleted the fix-finish-reason branch October 31, 2023 15:57

heyselbi mentioned this pull request Nov 17, 2023

Caikit Standalone image/SR opendatahub-io/caikit-tgis-serving#163

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return correct finish reasons in generate_text_func #210

return correct finish reasons in generate_text_func #210

dtrifiro commented Sep 28, 2023 •

edited

Loading

dtrifiro commented Sep 28, 2023

dtrifiro commented Sep 28, 2023 •

edited

Loading

Xaenalt commented Sep 28, 2023

gabe-l-hart commented Sep 28, 2023

gkumbhat left a comment

gkumbhat Sep 29, 2023

dtrifiro Sep 29, 2023 •

edited

Loading

gkumbhat Sep 29, 2023

dtrifiro Oct 2, 2023

gkumbhat commented Sep 29, 2023

dtrifiro commented Sep 29, 2023 •

edited

Loading

dtrifiro commented Oct 7, 2023

gkumbhat Oct 10, 2023

dtrifiro Oct 27, 2023 •

edited

Loading

gkumbhat Oct 27, 2023

dtrifiro Oct 27, 2023

dtrifiro Oct 30, 2023

gkumbhat left a comment

dtrifiro commented Oct 31, 2023

return correct finish reasons in generate_text_func #210

return correct finish reasons in generate_text_func #210

Conversation

dtrifiro commented Sep 28, 2023 • edited Loading

Reproduction script

dtrifiro commented Sep 28, 2023

dtrifiro commented Sep 28, 2023 • edited Loading

Xaenalt commented Sep 28, 2023

gabe-l-hart commented Sep 28, 2023

gkumbhat left a comment

Choose a reason for hiding this comment

gkumbhat Sep 29, 2023

Choose a reason for hiding this comment

dtrifiro Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

gkumbhat Sep 29, 2023

Choose a reason for hiding this comment

dtrifiro Oct 2, 2023

Choose a reason for hiding this comment

gkumbhat commented Sep 29, 2023

dtrifiro commented Sep 29, 2023 • edited Loading

dtrifiro commented Oct 7, 2023

gkumbhat Oct 10, 2023

Choose a reason for hiding this comment

dtrifiro Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

gkumbhat Oct 27, 2023

Choose a reason for hiding this comment

dtrifiro Oct 27, 2023

Choose a reason for hiding this comment

dtrifiro Oct 30, 2023

Choose a reason for hiding this comment

gkumbhat left a comment

Choose a reason for hiding this comment

dtrifiro commented Oct 31, 2023

dtrifiro commented Sep 28, 2023 •

edited

Loading

dtrifiro commented Sep 28, 2023 •

edited

Loading

dtrifiro Sep 29, 2023 •

edited

Loading

dtrifiro commented Sep 29, 2023 •

edited

Loading

dtrifiro Oct 27, 2023 •

edited

Loading