[cuebot/cuegui/pycue/rqd] Allow delaying a layer start - #2502
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds operator-controlled and automatic deferred layer booking across Cuebot, the scheduler, gRPC, pycue, CueGUI, metrics, and documentation. It also adds live RQD exit-status rule reloads for running frames and updates the project version to ChangesDeferred layer booking
Live RQD exit-status rules
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant FrameCompleteHandler
participant LayerDaoJdbc
participant PrometheusMetricsCollector
Dispatcher->>FrameCompleteHandler: report frame completion status
FrameCompleteHandler->>LayerDaoJdbc: extend layer start-after gate
FrameCompleteHandler->>PrometheusMetricsCollector: record delay event
sequenceDiagram
participant RQD
participant ConfigWatcher
participant RunningFrame
RQD->>ConfigWatcher: start periodic rule reload
ConfigWatcher->>ConfigWatcher: read and compile exit-status rules
ConfigWatcher->>RunningFrame: publish shared rule snapshot
RunningFrame->>RunningFrame: scan logs with current rules
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
cuegui/cuegui/MenuActions.py (1)
985-987: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the shared action metadata explicit.
Ruff flags
setStartAfter_infoas a mutable class attribute. Add the project-compatibleClassVarannotation, or use an immutable metadata type ifAbstractActionssupports it. This makes the shared class-level state intentional and clears RUF012 without changing the action contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cuegui/cuegui/MenuActions.py` around lines 985 - 987, Update the setStartAfter_info class attribute in AbstractActions with the project-compatible ClassVar annotation, preserving its current metadata values and action contract; use an immutable metadata type only if already supported by the class.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.java`:
- Line 79: Apply the existing ts_start_after eligibility predicate to the layer
subqueries used by FIND_JOBS_BY_LOCAL, FIND_UNDER_PROCED_JOB_BY_FACILITY, and
HIGHER_PRIORITY_JOB_BY_FACILITY_EXISTS, so delayed layers are excluded before
candidate selection. Add coverage verifying local booking and preemption
candidate queries ignore layers whose start-after timestamp is in the future.
In `@cuebot/src/main/java/com/imageworks/spcue/dao/postgres/LayerDaoJdbc.java`:
- Around line 728-737: The DELAY_LAYER_FOR_BACKOFF update must collapse
equal-delay requests across independent transactions, so update
LayerDaoJdbc.delayLayerForBackoff to compare against the persisted start time
using a transaction-independent database expression rather than
transaction-fixed current_timestamp. In LayerDaoTests, run the equal-delay calls
in separate transactions and verify the later call does not move the delay.
In `@cuebot/src/main/java/com/imageworks/spcue/dispatcher/LayerDelayRules.java`:
- Around line 58-72: In LayerDelayRules.java lines 58-72, update the parsing
catch around Duration.ofMinutes in LayerDelayRules so ArithmeticException is
handled together with NumberFormatException and the entry is skipped as
malformed. In LayerDelayRulesTests.java lines 35-92, add a rule with an
overflowing minute value and assert it is ignored while existing valid entries
continue to parse.
In
`@cuebot/src/main/resources/conf/ddl/postgres/migrations/V47__Add_layer_start_after.sql`:
- Around line 11-12: Update the V47 migration’s index statement to use
concurrent creation, and configure Flyway 9 for non-transactional execution plus
the Flyway 5.2.0 test setup for mixed or non-transactional migrations so the
statement does not run inside a transaction.
In `@cuegui/cuegui/LayerMonitorTree.py`:
- Around line 425-426: Escape rpcObject.data.start_after_reason in the
LayerMonitorTree tooltip handling for QtCore.Qt.ToolTipRole and
COLUMN_START_AFTER before returning it, so markup-shaped input renders verbatim
while preserving the existing tooltip behavior.
In `@docs/news/2026-08-07-layer-start-after-deferred-booking.md`:
- Around line 10-12: Update the heading hierarchy in the document so the title
is followed by an H2 section heading and its nested date heading uses H3,
preserving the existing heading text and document structure.
In `@proto/src/job.proto`:
- Around line 735-738: Change the public epoch-second fields start_after and its
corresponding start_after_reason-related timestamp representation to int64, then
update the Java handlers, response mappers, and callers that currently parse,
store, or convert these values as signed int32. Preserve the zero-as-unset
behavior and ensure future epoch-second timestamps remain accurate end to end
before regenerating clients.
In `@pycue/opencue/wrappers/layer.py`:
- Around line 753-755: Update the start-after accessor around the formatted
return so a zero or absent self.data.start_after returns an empty string when
format is supplied, while preserving the raw zero-value behavior when no format
is supplied. Add tests covering both raw and formatted unset values.
- Line 733: Rename the public format parameter in startAfter to time_format (or
another non-shadowing name), and update every local reference and call-site
usage in that method while preserving its behavior.
---
Nitpick comments:
In `@cuegui/cuegui/MenuActions.py`:
- Around line 985-987: Update the setStartAfter_info class attribute in
AbstractActions with the project-compatible ClassVar annotation, preserving its
current metadata values and action contract; use an immutable metadata type only
if already supported by the class.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a98e68b3-5d5e-45da-a501-b1e619a031cc
📒 Files selected for processing (45)
VERSION.incuebot/src/main/java/com/imageworks/spcue/LayerDetail.javacuebot/src/main/java/com/imageworks/spcue/PrometheusMetricsCollector.javacuebot/src/main/java/com/imageworks/spcue/dao/LayerDao.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/FrameDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/LayerDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dao/postgres/WhiteboardDaoJdbc.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/FrameCompleteHandler.javacuebot/src/main/java/com/imageworks/spcue/dispatcher/LayerDelayRules.javacuebot/src/main/java/com/imageworks/spcue/servant/ManageLayer.javacuebot/src/main/resources/conf/ddl/postgres/migrations/V47__Add_layer_start_after.sqlcuebot/src/main/resources/conf/spring/applicationContext-service.xmlcuebot/src/main/resources/opencue.propertiescuebot/src/test/java/com/imageworks/spcue/test/dao/postgres/FrameDaoTests.javacuebot/src/test/java/com/imageworks/spcue/test/dao/postgres/LayerDaoTests.javacuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerFrameStateTests.javacuebot/src/test/java/com/imageworks/spcue/test/dispatcher/FrameCompleteHandlerLayerDelayTests.javacuebot/src/test/java/com/imageworks/spcue/test/dispatcher/LayerDelayRulesTests.javacuegui/cuegui/DarkPalette.pycuegui/cuegui/LayerDialog.pycuegui/cuegui/LayerMonitorTree.pycuegui/cuegui/MenuActions.pydocs/_docs/reference/rust-rqd.mddocs/news/2019-04-18-season-of-docs-2019.mddocs/news/2019-07-08-opencue-birds-of-a-feather-at-siggraph.mddocs/news/2019-07-22-opencue-steering-committee-at-siggraph.mddocs/news/2019-09-20-opencue-at-siggraph-recording.mddocs/news/2019-12-05-la-pipeline-developers-meetup.mddocs/news/2019-12-18-sony-pictures-imageworks-case-study.mddocs/news/2020-08-27-google-summer-of-code-20-cloud-plugin.mddocs/news/2021-08-04-open-source-days-2021.mddocs/news/2024-05-24-opencue-project-review-2024.mddocs/news/2025-08-10-opencue-project-review-2025.mddocs/news/2025-12-12-distributed-scheduler-release.mddocs/news/2026-01-21-opencue-major-releases-2026-roadmap.mddocs/news/2026-07-07-cueweb-full-cuegui-parity-release.mddocs/news/2026-08-06-rqd-log-exit-status-rules.mddocs/news/2026-08-07-layer-start-after-deferred-booking.mdproto/src/job.protopycue/opencue/wrappers/layer.pypycue/tests/wrappers/test_layer.pyrust/crates/scheduler/src/dao/frame_dao.rsrust/crates/scheduler/src/dao/layer_dao.rsrust/crates/scheduler/tests/layer_start_after.rs
ramonfigueiredo
left a comment
There was a problem hiding this comment.
Approved with some suggested changes
Read docs/news/2026-08-07-layer-start-after-deferred-booking.md for more information. The original intent of this feature was to give frames that failed with license errors unlimited backoff retries. This prevents license shortages from causing a storm of dead frames on the farm that require manual intervention. To accomplish that a new feature was added where a layer can be set to start at a specific time, which allows the FrameComplete logic to postpone the retry of frames based on their exitStatus.
3946798 to
1045f56
Compare
99e0140
into
AcademySoftwareFoundation:master
## Related Issues - Related PR: #2502 - #1800 ## Summarize your change. Bring CueWeb to parity with the CueGUI half of the layer start-after gate: the time before which no frame of a layer may be booked, written either by an operator or automatically by Cuebot's exit-status backoff (dispatcher.layer_delay.rules). - Layers table gains a sortable "Start After" column. The proto field is an int64, so the gateway marshals it as a JSON string the way it already does for minMemory/maxRss; layerStartAfterSeconds() normalizes string | number | undefined in one place and the column's accessor sorts on that number rather than on the formatted date. - Delayed rows are tinted via SimpleDataTable's getRowClassName hook - the same mechanism the Hosts table uses - on both the job detail page and the inline job panel. The tint compares against the current time, so it clears itself on the first render after the deadline passes. - "Set Start After..." in the layer context menu and on layer nodes in the Job Dependency Graph opens a dialog with the same +15m / +1h / +4h / Tonight 18:00 presets as CueGUI. The picker reads and writes local time and the RPC carries UTC epoch seconds; Clear sends 0. - New /api/layer/action/setstartafter proxies to /job.LayerInterface/SetStartAfter and rejects a non-integer, negative, or more-than-five-years-out value before it reaches the gateway, mirroring the INVALID_ARGUMENT Cuebot raises for a milliseconds-for-seconds mistake. The signed-in username travels with the request and is stored as the layer's start-after reason, which is rendered as plain text in both the column tooltip and the dialog. rest_gateway needs no registration change - it generates a route per RPC from job.proto, so SetStartAfter appears once the image is rebuilt. The news post is corrected accordingly; it had listed both that and CueWeb parity as open follow-up work. ## LLM usage disclosure Parts of this solution's implementation were developed with assistance from Claude Opus. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## New Features - Added deferred layer scheduling through **Set Start After…** actions in layer menus and dependency-graph nodes. - Added scheduling presets, local-time editing, delay clearing, reason display, and attribution. - Added a sortable **Start After** column and visual highlighting for delayed layers. - Added validation, success/error feedback, and safety controls for restricted job interactions. ## Documentation - Updated user and developer documentation with scheduling behavior, controls, validation, and display details. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Brings in AcademySoftwareFoundation#2473, AcademySoftwareFoundation#2497, AcademySoftwareFoundation#2501, AcademySoftwareFoundation#2502, AcademySoftwareFoundation#2504, AcademySoftwareFoundation#2506, AcademySoftwareFoundation#2508, AcademySoftwareFoundation#2510, AcademySoftwareFoundation#2511 and the cueweb dependency bumps. Conflict resolutions: - applicationContext-service.xml, FrameDao: keep both sides. - DispatchQuery: keep the branch's layer.pk_layer fix plus master's ts_start_after predicate. - RqdClientGrpc: master's split catch and unknown-outcome throw, keeping the branch's warn log of the underlying launch failure on both paths. - FrameCompleteHandler: master refactored the class into small methods, so the scheduler hooks were re-applied on top of the new structure rather than merged line by line. The drain path (resolveForDrain) now also runs master's orphan-finalize and run-ownership fences, and handleStaleCompletion delegates to handleStaleReport so scheduler-owned shows get the same duplicate check and re-read guard as legacy ones; its Scheduler call site passes the report. The bookingOff guards became isBookingOff() calls inside the extracted booking methods, and the per-frame OOM bump moved into retryFrameWithRaisedMemory. FrameCompleteHandlerOwnershipTests builds the handler from a bare Environment mock; the constructor now unboxes numeric properties, so the mock returns the supplied defaults. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114CS5RrhDjTJse5faC4Pcb
Read docs/news/2026-08-07-layer-start-after-deferred-booking.md for more information.
The original intent of this feature was to give frames that failed with license errors unlimited backoff retries. This prevents license shortages from causing a storm of dead frames on the farm that require manual intervention.
To accomplish that a new feature was added where a layer can be set to start at a specific time, which allows the FrameComplete logic to postpone the retry of frames based on their exitStatus.
A more detailed explanation written by Claude:
The Challenge
A license shortage is not a property of the frame that hit it. Previously Cuebot could not tell one
from a generic crash, so a farm at license cap produced a wave of dead frames that someone had to
clean up by hand, every single time. Retrying immediately is pointless — the license is still gone —
and
maxRetriesis typically low enough that the frames died within seconds of each other.RQD's log-based exit-status rules gave RQD the ability to recognise the failure from the frame log
and report a substitute exit status. This feature is the Cuebot half: react to that status by
pausing the layer for a few minutes rather than killing its frames. Pausing the layer is correct
because every frame in a layer depends on the same license — one failure is enough to establish
that the pool is exhausted.
The Solution
Automatic backoff configuration
Add matching configuration to both sides:
rqd.yaml(see the Rust RQD reference):opencue.properties:When a frame exits
330, Cuebot marks the frameWAITING(no retry consumed), recordsAutomatic backoff: exit status 330on the layer, and defers the layer's booking for 5 minutes.In-flight frames on the same layer reporting the same status collapse into that one write. When the
delay expires the layer books again; if the license is still gone, the next report re-delays it.
330is the conventional license-shortage code — it sits safely outside the exit statuses Cuebotreserves internally.
Key behaviors:
their full retry budget for genuine failures.
finishes promptly.
operator-set 18:00 start cannot be pulled earlier by a backoff, and a longer rule can extend a
shorter active delay.
Rust scheduler both enforce it at the frame-reservation update, the single authoritative choke
point.
LLM usage disclosure
Claude Opus was used for implementing the changes on this PR after presented with a detailed design written by me.
Summary by CodeRabbit