-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
core: Add signal/timeout options to RunnableConfig #6305
Conversation
- Handled by all built-in runnables - Handled by all utility methods in base runnable, which should propagate to basically all runnables
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
langchain-core/src/runnables/base.ts
Outdated
this, | ||
inputs, | ||
optionsList, | ||
runManagers, | ||
batchOptions | ||
); | ||
outputs = optionsList?.[0]?.signal |
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.
Nice
langchain-core/src/runnables/base.ts
Outdated
output = options?.signal | ||
? await Promise.race([ | ||
promise, | ||
new Promise<never>((_, reject) => { |
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, edge case but what if someone passes an already aborted signal/there's some race condition?
Feel like there should be a check?
langchain-core/src/utils/signal.ts
Outdated
if (signal === undefined) { | ||
return promise; | ||
} | ||
if (signal.aborted) { |
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.
@nfcampos If we have this check here, do we need all the signal.throwIfAborted()
calls elsewhere?
…nc/31jul/runnable-abort-signal
…nc/31jul/runnable-abort-signal
…to nc/31jul/runnable-abort-signal
Fixes # (issue)