-
Notifications
You must be signed in to change notification settings - Fork 47
Add a new retry feature to block
#824
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
Add a new retry feature to block
#824
Conversation
|
Hi @hirokuni-kitahara, the PR looks great. How about instead of adding 2 new fields we only have When you make changes to the AST, you can run: to automatically regenerate the jsonschema. This is probabaly what you did, but just double checking. There are also some other files that need to change (with an AST change): Finally, you can run the following locally to make sure everything is in good shape: and: See the contribution docs |
fe52fdc to
4ea3dde
Compare
|
Thank you for your feedback @vazirim ! |
|
@hirokuni-kitahara can you rebase (other PRs have also changed the schema) and then run |
|
Thank you that is a great feature! Instead of doing it only on I would also rename |
repeat blockblock
a4bd87d to
506e109
Compare
|
Thank you very much for the changes @hirokuni-kitahara! LGTM |
|
does this handle the case where an async/future'd model block invocation fails? |
mandel
left a comment
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.
It is great. I just added a couple of comments.
src/pdl/pdl_interpreter.py
Outdated
| raise exc from exc | ||
| if do_retry: | ||
| error = f"An error occurred in a PDL block. Error details: {err_msg}" | ||
| print(f"\n\033[0;31m[Retry {trial_idx+1}/{max_retry}] {error}\033[0m\n") |
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.
This could be printed on stderr.
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.
Thank you @mandel , I will fix this part.
src/pdl/pdl_interpreter.py
Outdated
| if do_retry: | ||
| error = f"An error occurred in a PDL block. Error details: {err_msg}" | ||
| print(f"\n\033[0;31m[Retry {trial_idx+1}/{max_retry}] {error}\033[0m\n") | ||
| scope = set_error_to_scope_for_retry(scope, error, block.pdl__id) |
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.
Why do you add the error to the trace instead of re-executing the block in the original scope?
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.
Thank you @mandel ,
I'm adding the error to the trace so that the agent's LLM can understand what went wrong in the previous trial. (I'm assuming the retry feature is being used within a ReAct-style loop.)
This allows the LLM to generate a different output from the model block in the next iteration.
However, I now realize there's also a need to support more traditional retry scenarios—such as retrying an HTTP connection—where adding the error to the trace might be excessive.
What do you think about adding a new block attribute like trace_error_on_retry as a boolean field? It could default to false and can be set to true for agent use-case.
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
2845347 to
baa673e
Compare
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
Signed-off-by: hirokuni-kitahara <[email protected]>
|
That's great. Thank you! |
Signed-off-by: hirokuni-kitahara [email protected]
This PR is based on the issue #823 and it adds two new fields
retryandtrace_error_on_retryto theblockclass.retryis an integer value which indicates the number of retry when some errors happen within the block. retry is enabled only when the value is specified and positive.trace_error_on_retryis a boolean value whether to add error information to the trace. This defaults to None (=False), but when it is set to True, errors during retry are added to the trace. This is useful for multiple trials withmodelblock to leverage self-reflection behavior of LLM.