No timeout by default, only for VLM PDF parsing for now #2172
Conversation
This file contains hidden or 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
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to e882ed6 in 2 minutes and 16 seconds. Click for details.
- Reviewed
103lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. py/core/base/providers/llm.py:57
- Draft comment:
Consider renaming the 'timeout' boolean parameter (e.g. to 'enable_timeout') for clarity. - Reason this comment was not posted:
Marked as duplicate.
2. py/core/base/providers/llm.py:115
- Draft comment:
In _execute_with_backoff_sync, note that when timeout is false, it bypasses the thread pool (and retries) by directly calling _execute_task_sync. Confirm if this change is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. py/core/parsers/media/pdf_parser.py:158
- Draft comment:
Explicitly enforcing 'timeout=True' for LLM calls in VLM PDF parsing is acceptable; ensure these timeouts are appropriate for all intended scenarios. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the timeouts are appropriate for all scenarios, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
4. py/core/parsers/media/pdf_parser.py:201
- Draft comment:
Timeout enforcement is added for non-anthropic LLM calls too; verify that timeout=True is desired in this branch as well. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. py/core/base/providers/llm.py:70
- Draft comment:
Raising built‐in TimeoutError after catching asyncio.TimeoutError may obscure original exception details. Consider either re-raising asyncio.TimeoutError or documenting this conversion. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. py/core/base/providers/llm.py:169
- Draft comment:
Document the new 'timeout' parameter in the aget_completion method to clarify its impact on call behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. py/core/parsers/media/pdf_parser.py:155
- Draft comment:
Timeout is now enforced (timeout=True) when calling aget_completion in VLM PDF parsing. Confirm that this behavior meets expected performance and error handling needs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the behavior meets expected performance and error handling needs. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.
8. py/core/parsers/media/pdf_parser.py:201
- Draft comment:
Timeout flag is set to True for non-anthropic parsing as well. Ensure that downstream error handling for timeouts in these calls provides clear feedback. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that downstream error handling provides clear feedback. This falls under the category of asking the author to ensure something is tested or behaves as intended, which violates the rules.
Workflow ID: wflow_yCFx2luoUT4v3H4g
You can customize 
py/core/base/providers/llm.py
Outdated
| async def _execute_with_backoff_async( | ||
| self, | ||
| task: dict[str, Any], | ||
| timeout: bool = False, |
There was a problem hiding this comment.
Consider renaming the boolean parameter timeout to something like apply_timeout for clarity on its purpose.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e1f87d1 in 58 seconds. Click for details.
- Reviewed
77lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. py/core/base/providers/llm.py:59
- Draft comment:
Renamed 'timeout' to 'apply_timeout' for clarity. Ensure all call sites and documentation are updated consistently. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all call sites and documentation are updated consistently, which violates the rule against asking the author to double-check things. The comment does not provide a specific suggestion or point out a specific issue with the code.
2. py/core/base/providers/llm.py:116
- Draft comment:
Updated '_execute_with_backoff_sync' to use 'apply_timeout' instead of 'timeout'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating what was changed without providing any suggestion, question, or issue to address. It doesn't ask for confirmation or suggest any improvements.
3. py/core/base/providers/llm.py:170
- Draft comment:
Parameter name changed in aget_completion; using 'apply_timeout' for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating that a parameter name was changed for consistency. It does not provide a suggestion, ask for confirmation, or point out a potential issue.
4. py/core/parsers/media/pdf_parser.py:158
- Draft comment:
Updated call to aget_completion: replaced 'timeout=True' with 'apply_timeout=True' for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states what was changed without providing any suggestion, question, or request for confirmation. It doesn't align with the rules for useful comments.
5. py/core/parsers/media/pdf_parser.py:201
- Draft comment:
Updated call to aget_completion: replaced 'timeout=True' with 'apply_timeout=True' for consistency in the non-anthropic branch. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change that was made without suggesting any action or asking for clarification. It doesn't provide any actionable feedback or raise any concerns about the code.
6. py/core/base/providers/llm.py:59
- Draft comment:
Renamed 'timeout' to 'apply_timeout' for clarity. Consider updating docstrings to reflect this change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. py/core/parsers/media/pdf_parser.py:155
- Draft comment:
Updated parameter name from 'timeout=True' to 'apply_timeout=True' for consistency with llm methods. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. py/core/parsers/media/pdf_parser.py:404
- Draft comment:
The condition to check for halfwidth and fullwidth forms is split across multiple lines (lines 404-406). It would be clearer to write this as a single continuous comparison: "or "\uff00" <= x <= "\uffef"". This improves readability and reduces potential confusion. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_wQDvoaZIMunUV0A5
You can customize
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 optional timeout feature for VLM PDF parsing in
VLMPDFParserusingapply_timeoutparameter inllm.py.apply_timeoutparameter in_execute_with_backoff_asyncand_execute_with_backoff_syncinllm.pyto optionally apply timeouts.VLMPDFParserinpdf_parser.pynow usesapply_timeout=Truewhen callingaget_completionfor PDF page processing.TimeoutErrorinstead ofExceptionon request timeout inllm.py.llm.pyto handle optional timeout logic.This description was created by
for e1f87d1. You can customize this summary. It will automatically update as commits are pushed.