-
Notifications
You must be signed in to change notification settings - Fork 826
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
get rid of observer_cruise in observer_service code #1651
Conversation
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Replaces `observer_cruise` with `observer_task` in `observer_service.py` and adds `ObserverCruiseNotFound` exception for missing tasks. > > - **Behavior**: > - Replaces `observer_cruise` with `observer_task` in `run_observer_cruise()` and `run_observer_cruise_helper()` in `observer_service.py`. > - Introduces `ObserverCruiseNotFound` exception in `exceptions.py` for missing observer tasks. > - **Functions**: > - Updates return type of `run_observer_cruise()` and `mark_observer_cruise_as_failed()` to `ObserverTask`. > - Modifies `run_observer_cruise_helper()` to return a tuple including `ObserverTask`. > - **Misc**: > - Adjusts logging and error handling to use `observer_task` terminology in `observer_service.py`. > > <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 cdb91dc5a2610b7f49ad2d36c6bdf7ed4735487a. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
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 0dfd4fe in 14 seconds
More details
- Looked at
423
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/exceptions.py:578
- Draft comment:
Consider renamingobserver_cruise_id
toobserver_task_id
in the exception message for consistency with the rest of the code changes. - Reason this comment was not posted:
Confidence changes required:50%
The PR aims to replace 'observer_cruise' with 'observer_task' in the observer_service code. However, the exception class 'ObserverCruiseNotFound' still uses 'observer_cruise_id' in its message, which should be updated to 'observer_task_id' for consistency.
Workflow ID: wflow_qDnuSqwJmQlPHHzZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 0dfd4fe in 32 seconds
More details
- Looked at
423
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/exceptions.py:576
- Draft comment:
Consider renamingObserverCruiseNotFound
toObserverTaskNotFound
for consistency with the rest of the code changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment has a point - the error message uses "Observer task" not "Observer cruise". However, I don't have enough context about what an "observer cruise" vs "observer task" is. The class name may be intentionally using "cruise" to refer to a specific type of task. Without understanding the domain model, I can't be confident this rename would be correct.
I may be being too cautious - the error message clearly uses "task" which suggests the code author themselves thinks of this as a task. The inconsistency between message and class name could indicate a real issue.
While the message uses "task", I still can't be certain that "cruise" isn't the correct domain term. Making assumptions about domain terminology without full context is risky.
Without more context about what "observer cruise" means in this codebase, I cannot confidently say whether this rename would be correct, despite the inconsistent terminology in the error message.
Workflow ID: wflow_JhbV4k5fRVnit22w
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Renames
observer_cruise
toobserver_task
inobserver_service.py
and addsObserverCruiseNotFound
exception for missing tasks.observer_cruise
toobserver_task
inrun_observer_cruise
,run_observer_cruise_helper
, and related functions inobserver_service.py
.ObserverCruiseNotFound
exception inexceptions.py
to handle missing observer tasks.run_observer_cruise
andmark_observer_cruise_as_failed
toObserverTask
._summarize_observer_cruise
to returnObserverTask
.This description was created by for 0dfd4fe. It will automatically update as commits are pushed.