feat: thread instrument_capabilities through TaskData/TaskArchive/scheduler (§A.2-A.7) - #865
Merged
Merged
Conversation
…eduler (§A.2-A.7) - TaskData gains instrument_capabilities: InstrumentCapabilities | None (task.py) - TaskArchive gains get_instrument_capabilities() -> None by default; every non-portal backend inherits it unchanged (taskarchive.py) - Task.estimate_duration() gains an optional instrument_capabilities parameter, forwarded into TaskData - TaskScheduler.schedule() gains the parameter in its abstract signature. AstroplanScheduler accepts it for interface consistency but doesn't use it (reads the stored Task.duration field, not a live estimate -- see the plan's Non-goals). OnDemandScheduler threads it through the full call chain (schedule -> schedule_in_interval -> schedule_first_in_interval -> find_next_best_task/check_for_better_task/can_postpone_task/ create_scheduled_task -> the 4 task.estimate_duration() call sites) - pyobs/modules/robotic/scheduler.py's _schedule_worker, the only real caller of schedule() outside tests, supplies self._task_archive.get_instrument_capabilities() §A.2-A.7 of specs/plans/2026-09-01-instrument-capability-duration-estimates.md. PortalTaskArchive's concrete fetch/cache override (§A.4) and the 5 leaf scripts reading instrument_capabilities (§A.8) are follow-up work -- every backend still returns None today, so this PR changes no runtime behavior, only plumbing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
thusser
commented
Sep 2, 2026
thusser
left a comment
Member
Author
There was a problem hiding this comment.
Review: approve — clean, complete §A.2–A.7 plumbing
I traced the full call chain, checked interface containment across the repo and sibling projects, and verified the "no runtime behavior change" claim. No blockers; minor notes below.
Verified
- Threading is complete. All four
estimate_duration()call sites inpyobs/robotic/scheduler/ondemandscheduler.py(create_scheduled_task,evaluate_constraints_and_merits,check_for_better_task,can_postpone_task) receive the parameter, and the recursive chainschedule → schedule_in_interval → schedule_first_in_intervalthreads it in both directions, including the "between-tasks" recursion. Therun_cpu_boundpositional calls are consistent. - No behavior change — claim holds.
get_instrument_capabilities()defaults toNoneand every backend inherits it:FileSystemTaskArchive/Yaml,MemoryTaskArchive(tested),LcoTaskArchive, andPortalTaskArchive(future §A.4 override). Nothing reads the new field yet,AstroplanSchedulergenuinely ignores it, andTask.estimate_duration()without caps produces an identicalTaskDatato before. - Interface change is contained. Only two in-repo
TaskSchedulersubclasses exist (both updated) and exactly one real caller ofschedule()(pyobs/modules/robotic/scheduler.py's_schedule_worker). Sibling repos: no scheduler subclasses; pyobs-task-editor callstask.estimate_duration()zero-arg — unaffected. AllScript.estimate_durationoverrides take(data, time)positionally, so the unchanged script call convention holds. - Tooling: 119/119 tests pass across the four touched test files (see the IERS note below);
ruffandblackclean; no import cycles (smoke-imported every touched module). pyrefly reports 24 errors, all in the two pre-existing test files — confirmed identical ondevelop(same error set, line numbers shifted by the PR's insertions).
Notes (non-blocking)
- Out-of-tree
TaskSchedulersubclasses would break at runtime. Any site-specific scheduler (config class-string pattern) overridingschedule(tasks, projects, start, end)without the new kwarg raisesTypeErroronce driven by the updated module. §A.6 deliberately accepts this interface change and nothing in the checked sibling repos subclasses it — but a CHANGELOG/release note would be prudent. - One test is environment-dependent (not a code issue):
test_schedule_threads_instrument_capabilities_end_to_enddoes realObserver/time math and needs the astropy IERS table — it fails in a sandbox with a read-only~/.astropycache only, and passes withauto_max_age = None. Fine on networked CI; it just joins the existing IERS-dependent tests. - Nits:
_CapabilitiesEchoingScriptis duplicated acrosstest_task.pyandtest_ondemandscheduler.py(a conftest helper would DRY it); the module-level forwarding test'sfake_schedule+ CancelledError-by-sleep-count trick is clever but convoluted — acceptable since it matches the file's existing test patterns. - The relocated
# type: ignore[arg-type]on theschedule_first_in_intervalcall is still effective (pyrefly clean on the file).
Summary
Plumbing is complete, correct, and genuinely behavior-neutral; tests cover the forwarding at each meaningful layer (module → scheduler → create_scheduled_task, TaskData → script, archive default). Merge as-is and carry note 1 into the §A.4/A.8 follow-ups.
4 tasks
thusser
added a commit
that referenced
this pull request
Sep 3, 2026
…emented §A landed across pyobs-core#864/#865/#867/#868, released as pyobs-core v2.4.0. §B landed in pyobs-portal#144/#145 (dependency bumped, real path now exercised end-to-end, 0 skipped). All test-plan items checked off except the manual script-builder browser click-through, left open -- automated coverage exercises the same code path but nobody's watched it in a browser yet. specs/plans/index.md's entry marked implemented; fleet-open-items.md's plan entries removed per its own maintenance rule (implemented items don't stay, they get removed outright), #858/#859's dangling "plan below" reference reworded since the plan itself is no longer listed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
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
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.
Summary
§A.2-A.7 of
specs/plans/2026-09-01-instrument-capability-duration-estimates.md— the plumbing that carries anInstrumentCapabilities | None(from pyobs-core#864) from the one real caller down toTask.estimate_duration(), with no consumer yet and every backend still returningNone, so this changes no runtime behavior.TaskData.instrument_capabilities: InstrumentCapabilities | None(task.py)TaskArchive.get_instrument_capabilities() -> Noneby default; every non-portal backend (filesystem, memory, lco) inherits it unchangedTask.estimate_duration()gains the parameter, forwarded intoTaskDataTaskScheduler.schedule()gains it in the abstract signature.AstroplanScheduleraccepts it for interface consistency but doesn't use it (reads the storedTask.durationfield, not a live estimate — see the plan's Non-goals)OnDemandSchedulerthreads it through the full call chain:schedule→schedule_in_interval→schedule_first_in_interval→find_next_best_task/check_for_better_task/can_postpone_task/create_scheduled_task→ the 4task.estimate_duration()call sitespyobs/modules/robotic/scheduler.py's_schedule_worker— the only real caller ofschedule()outside tests — now suppliesself._task_archive.get_instrument_capabilities()Not in this PR
PortalTaskArchive's concrete fetch/cache override (still returnsNonevia the base class default)instrument_capabilitiesBoth tracked in the plan doc as follow-up work.
Test plan
TaskData/Task.estimate_duration()forwarding,TaskArchivedefault,OnDemandScheduler.create_scheduled_task()+ fullschedule()end-to-end threading,pyobs/modules/robotic/scheduler.py's_schedule_workerforwardingget_instrument_capabilities()pytest tests/(excludingintegration/xmpp) — 1866 passed, no regressionsruff check/black --check/pyrefly check— clean (pre-existing, unrelated pyrefly noise on 2 test files confirmed identical ondevelop)🤖 Generated with Claude Code
https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE