refactor(jobs): remove jobs-table dependencies from Python SDK (HYBIM-882)#109
refactor(jobs): remove jobs-table dependencies from Python SDK (HYBIM-882)#109etserend wants to merge 4 commits into
Conversation
…-882)
Port of upstream rungalileo/galileo-python commit d0dd159.
- Delete src/splunk_ao/job_progress.py and src/splunk_ao/jobs.py; both
relied on GET /jobs/{job_id} and GET /projects/{id}/runs/{id}/jobs
endpoints that the backend removed from the OpenAPI spec.
- Rewrite Experiment.monitor_progress() to poll experiment status
directly via get_status() + tqdm, removing the dependency on the
jobs table. The old job_id parameter is preserved as a deprecated
keyword-only argument that emits DeprecationWarning so callers
aren't broken silently.
- Delete tests/test_job_progress.py and tests/test_jobs.py.
- Add tests/test_experiment_progress.py with 6 tests for the new
polling-based monitor_progress() implementation.
- Remove Jobs.create patches from tests/test_experiments.py.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.
Verdict: request_changes — New polling loop has no failure/timeout exit, so a failed or stalled experiment polls forever — a regression from the old job_progress() which raised on failure.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
src/splunk_ao/shared/experiment_result.py:227-230:ExperimentStatusInfo.is_failedandExperimentPhaseInfo.is_failedare hardcoded to return False (TODO). Once the API exposes failure status, wiring these up would let monitor_progress and other consumers detect terminal failures properly.tests/test_experiment_progress.py:36-102: Test coverage omits the failure/stall path (understandably, since the current implementation can't terminate on it). Once failure handling/timeout is added, add a test asserting monitor_progress raises or times out rather than hanging on a non-completing experiment.
| while not status.is_complete: | ||
| new_progress = status.overall_progress | ||
| progress_bar.update(new_progress - progress_bar.n) | ||
| sleep(poll_interval_seconds) | ||
| status = self.get_status() | ||
| progress_bar.update(100 - progress_bar.n) |
There was a problem hiding this comment.
🟠 major (bug): This loop's only exit condition is log_generation progress reaching 100%. There is no failure detection and no timeout/max-attempts guard.
ExperimentStatusInfo.is_failed is a hardcoded stub (return False, see shared/experiment_result.py:227-230), and is_complete is purely progress_percent >= 100.0. So if the experiment fails, stalls, or the API never reports 100% (e.g. a completed experiment whose response lacks a log_generation phase, which ExperimentStatusInfo defaults to 0%), while not status.is_complete never terminates and this call hangs indefinitely, polling get_status() forever.
The old job_progress() explicitly raised ValueError on job failure — that safety net is gone. Please add a terminal-failure check and/or a bounded max-wait (e.g. a timeout_seconds param) so the loop can exit and surface an error instead of spinning forever.
🤖 Generated by the Astra agent
| return ExperimentStatusInfo(self._experiment_response) | ||
|
|
||
| def monitor_progress(self, job_id: str | None = None) -> str: | ||
| def monitor_progress(self, poll_interval_seconds: float = 2.0, *, job_id: str | None = None) -> None: |
There was a problem hiding this comment.
🟡 minor (design): Reordering makes job_id keyword-only while inserting poll_interval_seconds as the new first positional param. Existing callers that passed the job id positionally — experiment.monitor_progress("job-123") — will now silently bind that string to poll_interval_seconds (no DeprecationWarning, since job_id stays None), then fail later at sleep("job-123") with a TypeError. If preserving graceful deprecation for positional callers matters, consider detecting a str/UUID passed as poll_interval_seconds and routing it through the deprecation path, or at minimum note this hard break in the changelog/docstring.
🤖 Generated by the Astra agent
…ard to monitor_progress - Add timeout_seconds param (default 1h) so the polling loop exits with TimeoutError if the experiment never completes - Check status.is_failed each iteration and raise RuntimeError immediately instead of polling forever on a failed experiment - Detect a string passed positionally as poll_interval_seconds (legacy callers that used the old job_id positional param) and emit a DeprecationWarning instead of letting it fail later with TypeError - Add tests for all three new paths Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…n _make_status type(status).is_failed = property(...) mutated the ExperimentStatusInfo class itself, causing all subsequent instances in the same test session to return is_failed=True — breaking test_raises_timeout_error_when_deadline_exceeded. Switch _make_status to return a plain MagicMock with explicit attribute values so tests are fully isolated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mypy correctly rejects isinstance(poll_interval_seconds, str) when the parameter is typed as float — the check is statically unreachable. Drop the runtime guard and document the hard break in the docstring instead, as the reviewer suggested. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
etserend
left a comment
There was a problem hiding this comment.
Both issues addressed in commit 47b4f7b:
🟠 Major (infinite loop): Added timeout_seconds=3600.0 param and status.is_failed check. The loop now raises RuntimeError on failure and TimeoutError if the experiment stalls, matching the safety behaviour of the old job_progress().
🟡 Minor (positional string): Runtime guard was rejected by mypy (isinstance(float, str) is statically unreachable). Documented the hard break in the docstring instead, as suggested.
Summary
Port of upstream commit
d0dd159from rungalileo/galileo-python — third and final commit in the HYBIM-882 upstream migration.src/splunk_ao/job_progress.pyandsrc/splunk_ao/jobs.py— both relied onGET /jobs/{job_id}andGET /projects/{id}/runs/{id}/jobsendpoints that the backend removed from its OpenAPI spec. This is the root cause of the PR Update API Client #108 (Update API Client) CI failure.Experiment.monitor_progress()to poll experiment status directly viaget_status()+ tqdm progress bar, removing all jobs-table dependency. The oldjob_idparameter is kept as a deprecated keyword-only arg emittingDeprecationWarning.tests/test_job_progress.pyandtests/test_jobs.py.tests/test_experiment_progress.pywith 6 tests covering the new polling-based implementation.tests/test_experiments.pyto removeJobs.createpatches (no longer needed).Test plan
pytest tests/test_experiment_progress.py— 6/6 passpytest tests/test_experiments.py— 80/80 pass🤖 Generated with Claude Code