-
Notifications
You must be signed in to change notification settings - Fork 418
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
better catch exceptions #434
Conversation
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.
👍 Looks good to me! Reviewed everything up to d369a41 in 1 minute and 55 seconds
More details
- Looked at
160
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/forge/agent.py:433
- Draft comment:
Thefail_task
method does not handle the case where the task or step status cannot be updated to 'failed'. This could lead to situations where task failures are not properly recorded. Consider adding error handling or logging to address this potential issue. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_sunf7NSg7cPERcuz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
⌛ 1 day left in your free trial, upgrade for $20/seat/month or contact us.
skyvern/forge/agent.py
Outdated
api_key=api_key, | ||
close_browser_on_completion=close_browser_on_completion, | ||
) | ||
try: |
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.
oops i didn't see your PR until now. I already fixed the StepTerminationError handling in this PR: https://github.com/Skyvern-AI/skyvern-cloud/commit/358f04754bda789ebcc280132a95df84a869da7d#diff-97d27efc4f9823539ec219d1cc210dd6a7bf7d4c5ef4b1f9a1dfe937eff5774e
skyvern/forge/agent.py
Outdated
step_id=step.step_id, | ||
) | ||
finally: | ||
await self.send_task_response( |
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.
I don't think we should send_task_response again here since this is the exception handler for send_task_response
already. we might get into an infinite loop
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.
skyvern/forge/agent.py
Outdated
step_id=step.step_id, | ||
) | ||
finally: | ||
await self.send_task_response( |
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.
i think we should move this line to after line 412. it's very likely that the task is already in a timed_out or failed state. We might already have failed the task and sent a response to the webhook so we don't want to send webhook request again.
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 should be correct.
skyvern/forge/agent.py
Outdated
if isinstance(e, SkyvernException): | ||
failure_reason = f"unexpected SkyvernException({e.__class__.__name__})" | ||
|
||
try: |
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.
let's make the try except logic simpler
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.
❌ Changes requested. Incremental review on 2132c18 in 2 minutes and 18 seconds
More details
- Looked at
140
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_KtS9o7tfpINb1J0z
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
) | ||
return step, detailed_output, None | ||
|
||
async def fail_task(self, task: Task, step: Step | None, reason: str | None) -> None: |
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.
The PR description mentions introducing a retry mechanism, but the diff shows the removal of the retry mechanism from the fail_task
method. This change could lead to issues in handling transient errors effectively. Please clarify or reconsider this change.
2132c18
to
2030f65
Compare
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.
👍 Looks good to me! Incremental review on 2030f65 in 3 minutes and 34 seconds
More details
- Looked at
47
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_207FFFxDpEJo3Gw0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary:
This PR refactors exception handling, updates task status transitions, and adjusts the
fail_task
method by removing retry logic inskyvern/forge/agent.py
.Key points:
skyvern/forge/agent.py
fail_task
method inskyvern/forge/agent.py
, removing retry logicTaskStatus
transitions inskyvern/forge/sdk/schemas/tasks.py
to includefailed
fromqueued
Generated with ❤️ by ellipsis.dev