Skip to content

Show context length error msg #262

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.

Sign up for GitHub

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

Merged
yensung merged 8 commits into from
Apr 7, 2025
+21 −2
Merged

Show context length error msg #262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1cd282c
Show context length error msg
yensung Apr 2, 2025
33c6703
feedback
yensung Apr 3, 2025
5e6dcfd
Rename to bad connection error
yensung Apr 3, 2025
04fc211
Bad connection
yensung Apr 3, 2025
b7b20f4
revert
yensung Apr 4, 2025
8c10980
remove unused import
yensung Apr 4, 2025
d9d32ad
Add import
yensung Apr 4, 2025
c933298
Merge branch 'main' into 248ContextLengthError
yensung Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/inference/InferenceEngine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ class FinishReason(StrEnum):
# The model took longer than our timeout to return the first token
ModelOverloaded = "model overloaded"

# Encountered RPC error from inferD
BadConnection = "bad connection"

# Value error can be like when context length is too long
ValueError = "value error"

# General exceptions
Unknown = "unknown"

@dataclass
class InferenceEngineMessage:
Expand Down
15 changes: 13 additions & 2 deletions src/message/create_message_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,14 +392,21 @@ def map_chunk(chunk: InferenceEngineChunk):
yield map_chunk(chunk)

except grpc.RpcError as e:
finish_reason = FinishReason.BadConnection
err = f"inference failed: {e}"
yield format_message(message.MessageStreamError(reply.id, err, "grpc inference failed"))

except multiprocessing.TimeoutError:
finish_reason = FinishReason.ModelOverloaded
Copy link
Copy Markdown
Contributor

@mtblanton mtblanton Apr 2, 2025

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Copy Markdown
Contributor Author

@yensung yensung Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is changed to yielding error msg directly. I can change it back if you prefer the previous way

Copy link
Copy Markdown
Contributor

@mtblanton mtblanton Apr 2, 2025

Choose a reason for hiding this comment

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

Doesn't this affect how we save the message at the end?

Copy link
Copy Markdown
Contributor

@mtblanton mtblanton Apr 2, 2025

Choose a reason for hiding this comment

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

It does: https://github.com/allenai/olmo-api/pull/262/files#diff-372c652601d8e3f4422f10f598bb1bd952896faf1d09eb8f0529c4d412d5c3d0R477

Copy link
Copy Markdown
Contributor Author

@yensung yensung Apr 2, 2025

Choose a reason for hiding this comment

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

Good call. Curious why only TimeoutError exception is saved as finish_reason. Should we save RpcError and ValueError in finish_reason when they happen?

Copy link
Copy Markdown
Contributor

@mtblanton mtblanton Apr 3, 2025

Choose a reason for hiding this comment

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

ValueError may be a good finish reason, maybe something like FinishReason.InvalidRequest? I think RpcError is caused by a bad connection to InferD, so we may not need to save that?

Copy link
Copy Markdown
Contributor Author

@yensung yensung Apr 3, 2025

Choose a reason for hiding this comment

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

I'll name it to BadConnection

Copy link
Copy Markdown
Contributor

@mtblanton mtblanton Apr 3, 2025

Choose a reason for hiding this comment

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

You may want to keep that as-is or change the UI as well. We have some code tied to these values. https://github.com/allenai/olmo-ui/blob/dev/src/api/Message.ts#L99-L119

Copy link
Copy Markdown
Contributor Author

@yensung yensung Apr 4, 2025

Choose a reason for hiding this comment

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

I'll raise a PR to add them


gen = time_ns() - start_gen
gen //= 1000000
except ValueError as e:
finish_reason = FinishReason.ValueError
# value error can be like when context length is too long
yield format_message(message.MessageStreamError(reply.id, f"{e}", "value error from inference result"))

except Exception as e:
finish_reason = FinishReason.Unknown
yield format_message(message.MessageStreamError(reply.id, f"{e}", "general exception"))

match finish_reason:
case FinishReason.UnclosedStream:
Expand Down Expand Up @@ -430,6 +437,10 @@ def map_chunk(chunk: InferenceEngineChunk):
# The generation is complete. Store it.
# TODO: InferD should store this so that we don't have to.
# TODO: capture InferD request input instead of our manifestation of the prompt format

gen = time_ns() - start_gen
gen //= 1000000

prompt = create_prompt_from_engine_input(chain)
output, logprobs = create_output_from_chunks(chunks)

Expand Down