-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Introduce Configurable Upper-Limits/default values for max retries and retry Interval in Error Handling(#12787) #12788
base: main
Are you sure you want to change the base?
Conversation
…HTTP node and tool node
…es on retry default settings related change
@@ -260,3 +260,31 @@ export const TEXT_GENERATION_TIMEOUT_MS = textGenerationTimeoutMs | |||
export const DISABLE_UPLOAD_IMAGE_AS_ICON = process.env.NEXT_PUBLIC_DISABLE_UPLOAD_IMAGE_AS_ICON === 'true' | |||
|
|||
export const FULL_DOC_PREVIEW_LENGTH = 50 | |||
|
|||
// System default upper bounds of max retry and retry intervals | |||
export const MAX_RETRIES_DEFAULT = 3 |
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 is a good idea. However I would recommend moving these parameters into docker-compose-template, see this below
dify/docker/docker-compose-template.yaml
Line 57 in 6e0fb05
TEXT_GENERATION_TIMEOUT_MS: ${TEXT_GENERATION_TIMEOUT_MS:-60000} |
We have an environment config regarding the timeout settings for frontend images, your approach won't take effect unless it is rebuilt again or start from the source code. By moving those into template will make sure they can be changed every time.
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.
@crazywoola
100% Agree, thanks for your review.
Let me do additional these changes. I will push those changes later once I implement it and test it.
Thanks!
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.
Summary
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Close #12787
Screenshots
Before change
Default retry config in dify cloud version
LLM
HTTP
Tool node
After change
to test if behavior with commited code is the same as that before the change
LLM
HTTP
left is to to check default, right is to to check upper limit
![image](https://private-user-images.githubusercontent.com/153587838/403697279-e060d151-ebfc-4eee-a4de-5c2ecdbf67f5.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzNTcyMjIsIm5iZiI6MTczOTM1NjkyMiwicGF0aCI6Ii8xNTM1ODc4MzgvNDAzNjk3Mjc5LWUwNjBkMTUxLWViZmMtNGVlZS1hNGRlLTVjMmVjZGJmNjdmNS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjEyJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxMlQxMDQyMDJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0xZmFkN2YwODc0OTIwZmRkNDQ3NGI2Yzc5NjU5MzIwMDEyN2NmMWIzYjZjZWYwODc3NjQwOGE1NmE3YWVjYWFmJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.uOMulvEMXnldrnOiFJeBKLjCXeAxTOi0I5eq4TSve0U)
Tool node
to test if config file works as expected
LLM
HTTP
Tool
Checklist
Important
Please review the checklist below before submitting your pull request.
dev/reformat
(backend) andcd web && npx lint-staged
(frontend) to appease the lint gods