fix build boundingbox timeout #1646
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Add timeout to `evaluate` calls in `remove_bounding_boxes` and `build_elements_and_draw_bounding_boxes` in `page.py`. > > - **Behavior**: > - Add `timeout_ms=BUILDING_ELEMENT_TREE_TIMEOUT_MS` to `evaluate` calls in `remove_bounding_boxes()` and `build_elements_and_draw_bounding_boxes()` in `page.py`. > - **Misc**: > - No other changes or refactors were made. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 7dbbe119e1fb2f7a704cdc18d505dbd91d2eca2d. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d98e777 in 8 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/webeye/utils/page.py:221
- Draft comment:
Consider making the timeout value configurable or context-specific instead of using a hardcoded value for all cases. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a timeout parameter to the evaluate function calls for two methods. This is a good practice to prevent indefinite waiting. However, the timeout value is hardcoded as BUILDING_ELEMENT_TREE_TIMEOUT_MS, which might not be suitable for all cases. It would be better to allow this to be configurable or to use a more context-specific timeout value.
Workflow ID: wflow_L6I0df8YJeRlTsl7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to d98e777 in 8 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/webeye/utils/page.py:221
- Draft comment:
Ensure thatBUILDING_ELEMENT_TREE_TIMEOUT_MSis an appropriate timeout value for the operations inremove_bounding_boxesandbuild_elements_and_draw_bounding_boxes. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a timeout parameter to the evaluate function calls in remove_bounding_boxes and build_elements_and_draw_bounding_boxes methods. This is a good practice to prevent indefinite waiting, but the timeout value should be checked for appropriateness.
Workflow ID: wflow_DliACa1yVRE74jy4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
LawyZheng
approved these changes
Jan 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Add timeout to
remove_bounding_boxes()andbuild_elements_and_draw_bounding_boxes()inpage.pyusingBUILDING_ELEMENT_TREE_TIMEOUT_MS.timeout_ms=BUILDING_ELEMENT_TREE_TIMEOUT_MStoevaluate()calls inremove_bounding_boxes()andbuild_elements_and_draw_bounding_boxes()inpage.py.BUILDING_ELEMENT_TREE_TIMEOUT_MSfor timeout settings in the above functions.This description was created by
for d98e777. It will automatically update as commits are pushed.