Repository navigation
Conversation
An input passed explicitly to `Feature.__call__()` was kept in the
`_input` node forever, so a later call without input silently reused
it. This leaked into pipelines that reuse the feature as a property:
s = dt.Stack(value=2)
s(1)
(1 & s)() # [1, 1, 2] instead of [1, 2]
Simply writing `None` into `_input` when no input is given breaks two
existing behaviours: `Reducer(feature=...)` wires a feature directly as
`_input` (its value would be discarded), and a feature evaluated with
input and then as a property within the same pipeline resolution, as in
`inp - inp * 2`, must keep the input it just received.
Track resolution epochs instead. A module-level counter is incremented
on every top-level `Feature.__call__()` and each feature records, per
`_ID`, the epoch in which its input was set explicitly. When called
without input, an input set in the current epoch is kept, while one set
in a previous epoch is stale and the `_input` node is invalidated so
that it falls back to its action (an empty list, or the wired feature).
`update()` clears the records.
Update the `__call__` docstring, which documented the stale reuse, and
add a regression test.
Fixes DeepTrackAI#352
Follow-up to the epoch-based reset of stale explicit inputs. Key records by cache cell instead of by raw `_ID`. The `_input` cache trims an `_ID` longer than its key length to an existing cell, so two tuples can name one cell while the raw-`_ID` records disagreed about its epoch: `s(3); s(_ID=(0,))` and `s(3); Repeat(s, N=1)()` kept the stale `3`, and an input set during the current resolution through one alias could be discarded through another. Records now live in a `WeakKeyDictionary` keyed by the `DeepTrackDataObject` the cache resolves `_ID` to, so aliases share one record and records vanish with their cells when `update()` replaces the cache. `update()` therefore no longer needs an explicit clear, and components of a pipeline are purged as well. When a stale cell is reached only by trimming, the cache cannot store a recomputed value at that `_ID` next to its existing key length (the same limit already rejects `s(3); s(4, _ID=(0,))` on `develop`). The reset then resets the input and everything downstream, as `update()` does, instead of only invalidating. Keep the resolution depth and epoch in context variables, with epoch numbers drawn from one locked process-wide counter. Independent graphs evaluated from different threads no longer see each other's calls as nested, which made a top-level call share the epoch of a call blocked elsewhere and keep its stale input. Update the `Stack` docstring, which still showed the pre-fix output of the example from the issue. Tests: count `get()` calls through the explicit / empty / empty sequence and check a reducer falls back to its wired feature; cover `_ID` aliases, `Repeat`, the same-resolution alias case and per-cell independence; check records are purged for pipeline components after `update()`; check thread isolation with a deterministic blocked call.
The previous reset for a stale input reached through a trimmed `_ID` replaced the `_input` cache and everything downstream, as `update()` does. That rebuilt the caches with the alias's key length, so a later call at the original `_ID` failed in `DeepTrackDataDict.create_index` (`s(3); s(_ID=(0,)); s()`, or `(Repeat(s, N=1) & s)()` after `s(3)`, which returns on `develop`), and it also discarded sibling cells holding inputs received during the current resolution. The caches resolve a read at an `_ID` longer than their key length by trimming it, but reject a write there. So `Feature.__call__()` now trims such an `_ID` up front and uses the trimmed `_ID` throughout: resets, explicit inputs and outputs all land in the entry reads come from. A stale cell is then always reset with a plain per-cell `invalidate`, sibling cells keep their inputs, and the key length never changes. As a side effect, writes at a longer `_ID` that failed on `develop` now succeed (`s(3); s(5, _ID=(0,))`, a property invalidated and re-read at a longer `_ID`). An `_ID` shorter than the key length still fails at `store`, as on `develop`. Tests: `s()` after a reset through an alias; the `Repeat(s, N=1) & s` composition; a stale sibling reached through an alias next to a cell set in the current resolution; round trips between `_ID` lengths. The thread test now releases the blocked call in `finally`, checks both threads terminated and re-raises their exceptions.
Trimming `_ID` to the key length of the `_input` cache for the whole call also trimmed it for property sampling. After a targeted update such as `f.value.update()`, the property and output caches are empty but `_input` keeps its key length, so `f(_ID=(0,))` and `f(_ID=(1,))` sampled the same value and `Value(lambda _ID: _ID)` returned `()` for `(7,)`; both behave on `develop`. Resolve the trimmed `_ID` per cache instead, after sources have been activated: the `_input` operations use the `_ID` the `_input` cache holds the entry under, the output node is evaluated under the `_ID` its own cache holds it under, and properties and arguments keep the logical `_ID`. `action()` reads `_input` through the same resolution. Test: two distinct `_ID`s sample independently after a targeted update, with a deterministic counter and with an `_ID`-dependent value.
This branch has not been deployed
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.
Fixes #352.
Problem
An input passed explicitly to
Feature.__call__()stays in the_inputnode, so a later call without input reuses it:The second case is the one from the issue:
1 & sbuildsValue(1) >> Stack(value=s), andsis evaluated as a property, i.e. without input, so it picks up the1left over from the direct call.Why "no input ⇒ reset" is not enough
Writing
Noneinto_inputwhenever no input is given (the approach of #357) failstest_broadcast_list, and a variant that only resets inputs previously set by__call__failstest_Feature_arithmetic. Two existing behaviours get in the way:_inputcan be a computed node.Reducer(henceMean,Sum, …) acceptsfeature=and wires it as_input. Overwriting that node withNonediscards the feature's output, soinp - Mean(inp)ends up with[].inp - inp * 2the sameinpreceives10and is then re-evaluated as a property, without input, within the same resolution. It must keep the10. The issue is about an input left over from a previous call.Fix
Resolution epochs. A process-wide counter gives every top-level
Feature.__call__()a new epoch; nesting depth and current epoch live incontextvars.ContextVars, so independent threads or asyncio tasks do not see each other's calls as nested. A small context manager wraps the existingwith config.with_backend(...).Per-cell records. When a feature receives an input explicitly, it records the current epoch against the cache cell of
_inputthat_IDresolves to, in aWeakKeyDictionary. Keying by cell rather than by raw_IDmeans that_IDs the cache resolves to the same entry share one record, and that records disappear with their cells whenupdate()replaces the caches, for every component of a pipeline.Stale check. When called without input, the feature looks at the records of the cells
_IDrefers to. A record from the current epoch is kept (same-pipeline reuse, as ininp - inp * 2). A record from a previous epoch is stale: the record is dropped and the_inputcell is invalidated, so it falls back to its action (the default[], or the wired feature forReducer). Invalidating rather than overwriting is what keepsReducer(feature=...)working._IDresolution per cache. The caches resolve a read at an_IDlonger than their key length by trimming it, but reject a write there (DeepTrackDataDict.create_index).__call__therefore resolves, per cache, the_IDunder which that cache holds the entry:_inputoperations (explicit input, stale check, reset) use the_IDof the_inputcache, the output node is evaluated under the_IDof its own cache, and properties and arguments keep the logical_ID, so distinct_IDs still sample independently. This is what makes a feature called directly and then nested inRepeat(which calls it at_ID + (n,)) behave, and it keeps sibling cells and key lengths untouched. An_IDshorter than the key length still fails atstore, as ondevelop.Results:
Behaviour change to confirm
The
__call__docstring documented the reported behaviour as a feature:With this change,
feature()afterfeature(x)evaluates on an empty input ([]for a distributed feature), which is what the issue asks for. No test depended on the old behaviour. The__call__andStackdocstrings are updated. Flagging it since it is user-visible.Tests
New tests in
tests/test_features.py:inp - inp * 2);get()call counter for the explicit → empty → empty sequence (one call, one recomputation on[], then cached) and aMean(inp)falling back to its wired feature after an explicit input;_IDaliases: reset through a longer_IDthen a call at the original one,Repeat, theRepeat(s, N=1) & scomposition, the same-resolution alias case, per-cell independence, a stale sibling next to a cell set in the current resolution, round trips between_IDlengths;_IDs sampling independently after a targetedproperty.update(), with a deterministic counter and an_ID-dependent value;update();python -m unittest discover -s testson this revision: 520 tests, OK (129 skipped), numpy backend only (no PyTorch, OpenCV or Deeplay installed here). The two pytest collection errors (test_elementwise.py::test_method,test_features.py::test_operator) are pre-existing ondevelop: helper functions namedtest_*that pytest picks up.Notes
_inputof aReducer, which callsaction()directly) and only later throughFeature.__call__()without input, the stale reset triggers on that later call and re-evaluates the downstream nodes once more in the same epoch. Results are consistent; it is one extra computation in a rare pattern.Files
deeptrack/features.py: resolution scope, per-cell records, per-cache_IDresolution, stale check in__call__, docstrings of__call__andStack.tests/test_features.py: regression tests.