Fix every pytest warning, and make new ones fail the suite - #465
Conversation
`uv run pytest` printed four warnings and, intermittently, two stray SQLAlchemy
tracebacks after the summary line. Three distinct causes, three treatments.
1. Our own bug. `define_asset_job` deprecates `partitions_def` ("partitioning is
inferred from the selected assets, so setting this is redundant"), and
`defs/schedules.py` passed it at both partitioned call sites. Removed, along
with the two now-unused partition imports. The inference is not merely
equivalent-looking: `test_definitions_resolve` now asserts each resolved job's
`partitions_def` *is* the very object its asset declares, so a job silently
resolving to `None` — or to a different cadence or start — fails there rather
than at the next schedule tick.
2. Third-party, unfixable from here. Dagster 1.13.16's `sql_event_log.py` calls
the `datetime.utcfromtimestamp()` Python 3.12 deprecated. Silenced with a
`filterwarnings` entry pinned to both the exact message and the exact upstream
module, so it cannot mask a future warning from our own code, and commented
with the package and version to delete it at.
3. Teardown noise, not a warning. `DagsterInstance.ephemeral()` builds an
in-memory run storage and event-log storage, each holding one SQLAlchemy
connection to an in-memory SQLite database for the life of the instance.
Nothing closes them unless `dispose()` is called, and `DagsterInstance` has no
finaliser — so an instance handed to the garbage collector survives to
interpreter shutdown, where SQLAlchemy's pool finaliser can run after SQLite
has already closed the database and print `Exception during reset or similar`
/ `sqlite3.ProgrammingError: Cannot operate on a closed database`. Two of
them, one per held connection, which is exactly one leaked instance.
All 31 call sites were in `tests/` and each created exactly one instance per
test, so they now take a `dagster_instance` fixture that enters the instance
as a context manager and disposes it when the test ends. Measured with a
throwaway session-end probe over `tests/`: before, 6 live in-memory storages
with 6 open held connections; after, the same 6 storages with 0.
With the suite clean, `filterwarnings` gains a leading `error`, so a warning
introduced by our own code fails the PR that introduces it instead of
accumulating in the summary. The cost is that a dependency upgrade adding a new
upstream warning now fails loudly and needs an entry here — which is the
intended trade, and matches why the ruff rule selection is pinned. Nothing in
this repo calls `warnings.warn` or `pytest.warns`, so there is no intentional
warning for `error` to break. Verified green on the default suite and on the
network-gated test (`--run-network`), which `error` also covers.
395 passed, 1 skipped — no warnings summary, no stray tracebacks.
Closes #425
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…olicy Follow-up to the review on #465. Six changes, all narrowing or proving claims the first commit made loosely. - `pyproject.toml`: anchor both ignore entries with `$` so the module regex is an exact match rather than a prefix (`re.match` would otherwise also accept a hypothetical `…sql_event_log_v2`). Bring the pre-existing mlflow entry up to the standard the new comment block asserts: it had no version, no delete condition, an unescaped `.` in the message, and a whole-package `mlflow.*` anchor. Pinning it needed the actual call sites rather than the first one in the traceback — `codecs.open()` is called from *two* modules, `mlflow.utils.file_utils` and `mlflow.utils.yaml_utils`, so the anchor enumerates both. Verified by re-running the MLflow-backed tests: an anchor naming only `yaml_utils` fails five of them. - `tests/test_assets.py`: assert the *resolved schedule*, not just the job. `live_forecasts_schedule` is built by `build_schedule_from_partitioned_job`, so its cron is derived from the inferred `partitions_def` — the one thing dropping the explicit argument could plausibly have broken, and nothing asserted it. Also add `live_forecasts_job` to the asset-selection loop, which omitted it. - `tests/test_assets.py`: compare the inferred `partitions_def` with `==` rather than `is`. Identity is stronger than the property under test — the claim is that the job targets the same partitions, and Dagster is free to return an equal copy. `==` still catches `None` and any cadence or start difference, and is what Dagster's own inference uses internally. - `docs/architecture/testing.md`: document both new repo-wide rules, which were previously discoverable only from a docstring and a TOML comment — take the `dagster_instance` fixture rather than calling `DagsterInstance.ephemeral()`, and warnings are errors (with how to add an exception when an upgrade introduces an upstream warning). - `tests/conftest.py`: scope the fixture docstring's guidance to the test suite, and name the context-manager form as the equivalent outside it. - `tests/test_live_forecasts.py`: drop a blank line left at the top of a function body by the scripted refactor. `ruff format` does not remove it. 395 passed, 1 skipped; 396 passed with `--run-network`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review pass — what changed, and one thing deliberately left aloneAn adversarial review of the first commit raised six findings. Five are addressed in 2fdeb75; the Fixed
|
| bare call | with context manager | |
|---|---|---|
| happy path | 2 open | 0 open |
| error path | 2 open | 0 open |
So the context manager is not error-path insurance — it is the only thing that closes these
connections at all. docs/architecture/testing.md and the code comment were corrected in afb91df;
the code fix itself was right from the start.
Observable behaviour is unchanged: happy-path stdout is byte-identical, and on failure the exception
still propagates with exit code 1, skipping _report_metrics() and the closing "Done." line. The
other two Python scripts create no Dagster instance and no DB connection, so this was the only
remaining site.
Jack asked for the same undisposed-`DagsterInstance` fix in the baseline script
that the previous commits applied to the test suite.
Measuring first sharpened the diagnosis: the script's *happy* path was never
leaking. `instance` is a local of `main()`, so it is reclaimed by refcounting the
moment `main()` returns, and its two SQLite connections are finalised in a clean
order. The path that leaks is an **unhandled exception** — the traceback keeps
the raising frame alive, and with it the instance, all the way to interpreter
shutdown. That is exactly the window where SQLAlchemy's connection-pool finaliser
can run against an already-closed database and print the bare
`Exception during reset or similar` traceback.
An `atexit` probe counting in-memory storages whose held connection is still open
at interpreter exit, running `main()` with every expensive step stubbed so only
the instance lifecycle is real:
before after
happy path 0 open 0 open
error path 2 open 0 open
Two, not one, because the run storage and the event-log storage each hold one —
the same two-connection signature as the two tracebacks this all started from. On
the error path the storages are still *reachable* afterwards (the traceback owns
them, which is Python's semantics and not ours to change); what changes is that
their connections are closed, so the finaliser has nothing left to roll back. The
leak is closed, not moved.
The fix is `with DagsterInstance.ephemeral() as instance:`, whose `__exit__`
calls `dispose()` on both paths. The five pipeline steps move into
`_run_pipeline(instance)` rather than gaining a level of indentation, per the
"prefer small functions" rule in CLAUDE.md. Observable behaviour is unchanged:
happy-path stdout is byte-identical to the previous version, and on failure the
exception still propagates, the exit code is still 1, and neither `_report_metrics()`
nor the closing "Done." line runs.
The other two Python scripts create no Dagster instance and no DB connection, so
this was the only remaining site.
`docs/architecture/testing.md` gains the script-side idiom next to the fixture
rule. Note it is a sibling bullet, not an indented continuation paragraph: as a
continuation it rendered *outside* the `<ul>`, which `pymarkdown scan` does not
catch — the gotcha already recorded in CLAUDE.md. Verified in the built HTML.
395 passed, 1 skipped; 396 passed with `--run-network`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review of 53d717e caught a real error in that commit's reasoning, and the error had been written into a durable docs page, so it is worth stating plainly. 53d717e claimed a script's happy path was already safe — that `instance` is a local of `main()`, reclaimed by refcounting on return, and that only an unhandled exception could keep it alive to interpreter shutdown. That is false whenever the instance has actually been used. The measurement that produced it stubbed `materialize` out to a no-op, and stubbing `materialize` is precisely what removed the retention: with a real materialize, Dagster keeps the instance reachable for the rest of the process (a `RunDomain`, plus the partition-loading contexts that hold it as `dynamic_partitions_store` — confirmed with `gc.get_referrers`). Re-measured with one real `materialize` and no reference held outside the frame, counting storages whose held connection is still open at interpreter exit: bare with context manager happy path 2 open 0 open error path 2 open 0 open So the happy-path row of the previous commit's table was an artifact of the probe, and the fix is worth more than that commit claimed: it is not error-path insurance, it is the only thing that closes these connections at all. The code fix itself was already right and is unchanged. What changes here is the explanation in the two places that carried the wrong claim — `docs/architecture/testing.md`, which is durable guidance and would have told a reader that a script which cannot fail does not need the context manager, and the comment in `scripts/run_baseline_experiment.py`, now shortened to point at the docs rather than restate them, so the two cannot drift again. 395 passed, 1 skipped; 396 passed with `--run-network`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…_context() CI failed on PR #465 after all 395 tests passed: ExceptionGroup: multiple unraisable exception warnings (2 sub-exceptions) +-+---------------- 1 ---------------- ResourceWarning: unclosed database in <sqlite3.Connection object at 0x...> pytest.PytestUnraisableExceptionWarning: ... +---------------- 2 ---------------- ResourceWarning: unclosed database in <sqlite3.Connection object at 0x...> ... Two connections, escalated to a failure by the `filterwarnings = ["error"]` this PR added — the local `uv run pytest` run that reported "395 passed" clean does not contradict this: it means GC happened not to reclaim the leaking objects at the wrong moment on this machine that run, not that nothing was leaking. `tests/test_assets.py:295` calls `build_asset_context(partition_key="2024-05-01")` bare, inside `ecmwf_ens(...)`, inside a `pytest.raises(RetryRequested)` block. `build_asset_context()`'s docstring: "Defaults to `DagsterInstance.ephemeral()`" — built internally, owned by an `ExitStack` that only closes on `__exit__`, or otherwise on `__del__`. A bare call passed straight into the asset never gets an `__exit__`, so disposal depends on `__del__` running via garbage collection — and the traceback `pytest.raises` captures keeps the asset's frame, and the `context` argument in it, referenced past the end of that `with` block. Measured directly on the real file with a `pytest_runtest_teardown` hook and NO forced `gc.collect()` (matching what pytest's own unraisableexception plugin does at every item boundary): 2 connections open right after that one test's teardown, before the fix; 0, deterministically, after — matching the "2 sub-exceptions" CI reported exactly (one instance's run storage + event-log storage). The fix mirrors what `dagster_instance` and `run_baseline_experiment.py` already do: enter it as a context manager, `with build_asset_context(...) as context, pytest.raises(...) as exc_info:`, so `dispose()` runs at a fixed point in program order rather than whenever (if ever) the garbage collector gets to it. A repo-wide grep for every Dagster direct-invocation helper that can default-construct an instance (`build_asset_context`, `build_op_context`, `build_resources`, `build_schedule_context`, `build_sensor_context`) found exactly one call site outside the already-fixed `dagster_instance` fixture and `run_baseline_experiment.py`, and it was this one. `materialize()` and `JobDefinition.execute_in_process()`, both used elsewhere without an explicit `instance=`, do not have this problem: read from the Dagster source, both build their default instance behind their own internal `with ephemeral_instance_if_missing(instance):`, entered and exited before the function returns — deterministic regardless of how the caller uses the result. `docs/architecture/testing.md` records this distinction (the helper's own `with` block vs. one the caller must open) so the next Dagster call added to this repo can be checked against it directly, instead of rediscovering the mechanism. I could not reproduce the literal CI `ExceptionGroup` locally, despite trying the default suite, `python -X dev`, and a forced `gc.collect()` at `pytest_unconfigure` — all consistent with the framing that this depends on finalisation order within a single collection pass, which is allocator- and platform-dependent and does not always fire. What is reproduced, deterministically and on demand, is the underlying mechanism: the exact leaking objects, the exact window in which they are open and unreachable, and that the fix removes that window entirely rather than narrowing it. 395 passed, 1 skipped (× 4 consecutive local runs); 396 passed with `--run-network`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes #425.
uv run pytestprinted four warnings and, intermittently, two stray SQLAlchemy tracebacks afterthe summary line. Three distinct causes, three treatments.
1. Our own bug — fixed the code
define_asset_jobdeprecatespartitions_def("Partitioning is inferred from the selected assets,so setting this is redundant"), and
defs/schedules.pypassed it at both partitioned call sites(
ecmwf_ens_job,live_forecasts_job). Removed, along with the two imports that became unused.This is a behaviour change dressed as a deprecation fix, so it is asserted rather than assumed.
test_definitions_resolvenow checks that each resolved job'spartitions_defis the veryobject its asset declares:
A job silently resolving to
None, or to a different cadence or start, now fails there rather thanat the next schedule tick. I also diffed the whole resolved repository (every job's
partitions_defplus every schedule's cron, timezone and target job) before and after the change — byte-for-byte
identical, including
live_forecasts_job_schedule, which is derived viabuild_schedule_from_partitioned_joband so depends on the inference working.2. Third-party deprecation — narrowly suppressed
Dagster 1.13.16's
_core/storage/event_log/sql_event_log.py:3160calls thedatetime.utcfromtimestamp()that Python 3.12 deprecated. Not fixable from this repo, so it gets afilterwarningsentry pinned to both the exact message and the exact upstream module:The module anchor is the whole point — it means the entry can only ever match that one upstream
call and can never mask the same deprecation appearing in our own code. The comment names the
package and version and says to delete it once Dagster moves to timezone-aware datetimes.
3. Teardown noise — root cause found and fixed
Not a warning at all: two
Exception during reset or similartracebacks ending insqlite3.ProgrammingError: Cannot operate on a closed database, printed after pytest's summary.Root cause, and it is ours.
DagsterInstance.ephemeral()builds anInMemoryRunStorageand anInMemoryEventLogStorage. Each opens one SQLAlchemy connection against an in-memory SQLite databaseand holds it for the life of the instance (
self._held_conn = self._engine.connect()), and eachcloses it only in
dispose().DagsterInstancehas no finaliser, so an instance handed to thegarbage collector keeps both connections open until interpreter shutdown — where SQLAlchemy's
_finalize_fairyGC path can run after SQLite has already closed the underlying database, andlogs the traceback. Two tracebacks = two held connections = exactly one leaked instance.
All 31
DagsterInstance.ephemeral()call sites were intests/, and each test created exactly oneinstance, so they now take a
dagster_instancefixture that enters the instance as a contextmanager (
__exit__callsdispose()) and tears it down when the test ends.Evidence. A throwaway session-end probe counting live in-memory storages whose
_held_connisstill open, run over
tests/:Same instances still reachable from pytest's own caches either way; the difference is that they are
now disposed. Note the symptom itself is intermittent — it depends on finalisation order at
interpreter shutdown and did not reproduce in ~8 full runs on this machine — which is why the fix is
verified against the leak rather than against the traceback.
filterwarnings = ["error"]— enabledWith the suite clean,
filterwarningsgains a leadingerror, so a warning introduced by our owncode fails the PR that introduces it rather than accumulating in the summary. Reasoning:
warnings.warnorpytest.warns, so there is no intentional warningfor
errorto break.loudly and needs an entry here. That is the intended trade, and it matches why the ruff rule
selection is already pinned in this file — an upgrade should change behaviour visibly, not
silently.
errorgoes first and both ignores are applied on top of it.uv run pytest --run-network,which
erroralso covers and which is skipped by default, so it would otherwise go unchecked).Verification
No warnings summary, and no stray tracebacks after it.
🤖 Generated with Claude Code