diff --git a/alloc/models/networks.py b/alloc/models/networks.py index ca58ced..0dbcdf7 100644 --- a/alloc/models/networks.py +++ b/alloc/models/networks.py @@ -48,7 +48,10 @@ class ReplayBuffer: """ def __init__( - self, capacity: int, rng: np.random.Generator | None = None + self, + capacity: int, + rng: np.random.Generator | None = None, + log_interval: int = 1000, ) -> None: """Initialise the buffer with *capacity* slots. @@ -59,9 +62,14 @@ def __init__( rng : np.random.Generator, optional NumPy random generator for reproducible sampling. Defaults to ``np.random.default_rng()``. + log_interval : int, optional + Emit a DEBUG overflow log every *log_interval* overwrites + (default ``1000``). Must be ``>= 1``. """ if capacity <= 0: raise ValueError(f"capacity must be > 0, got {capacity}") + if log_interval < 1: + raise ValueError(f"log_interval must be >= 1, got {log_interval}") self._buffer: deque[ tuple[np.ndarray, np.ndarray, float, np.ndarray] ] = deque(maxlen=capacity) @@ -69,6 +77,8 @@ def __init__( self.rng: np.random.Generator = ( rng if rng is not None else np.random.default_rng() ) + self._overwrite_count: int = 0 + self._log_interval: int = log_interval logger.info("ReplayBuffer initialised with capacity=%d", capacity) def add( @@ -92,10 +102,13 @@ def add( Observation at time *t+1*. """ if len(self._buffer) == self._capacity: - logger.debug( - "ReplayBuffer full (capacity=%d), overwriting oldest entry", - self._capacity, - ) + self._overwrite_count += 1 + if self._overwrite_count % self._log_interval == 0: + logger.debug( + "ReplayBuffer full (capacity=%d), %d total overwrites", + self._capacity, + self._overwrite_count, + ) self._buffer.append((state, action, float(reward), next_state)) def sample( diff --git a/tests/test_ddpg_integration.py b/tests/test_ddpg_integration.py new file mode 100644 index 0000000..a55fdfe --- /dev/null +++ b/tests/test_ddpg_integration.py @@ -0,0 +1,109 @@ +"""Closed-loop DDPG training-step integration tests. + +These tests exercise the *real* training loop through the public API +(``get_allocation``) with a deterministic toy environment so that each +``next_state`` is a function of ``(state, action)`` — the closed-loop +dynamics the DDPG Bellman target assumes. They complement the synthetic +smoke test in ``test_actor_critic.py::TestDDPGTrainingStep``. +""" + +from __future__ import annotations + +import math + +import numpy as np +import pytest + +from alloc.models.networks import ActorCriticNetworks + +INPUT_DIM = 10 +NUM_ASSETS = 5 +CAPACITY = 8 + + +def _env_step(state: np.ndarray, action: np.ndarray) -> tuple[np.ndarray, float]: + """Deterministic toy dynamics. + + ``next_state`` depends on both the current state and the chosen action, + and the reward is tied to the allocation so the critic has a signal to + learn from. + """ + action = np.asarray(action, dtype=np.float64) + state = np.asarray(state, dtype=np.float64) + # Blend the state decay with the action's influence on each asset. + next_state = 0.9 * state + 0.1 * np.repeat(action, INPUT_DIM // NUM_ASSETS) + reward = float(np.dot(action, state[:NUM_ASSETS])) + return next_state, reward + + +@pytest.fixture() +def networks() -> ActorCriticNetworks: + return ActorCriticNetworks( + input_dim=INPUT_DIM, + num_assets=NUM_ASSETS, + seed=42, + min_cash_allocation=0.05, + buffer_capacity=CAPACITY, + ) + + +def test_closed_loop_training_step(networks: ActorCriticNetworks) -> None: + """A full closed-loop training step through the public API is finite.""" + n_steps = 16 # > CAPACITY so the buffer overflows + state = np.random.default_rng(0).standard_normal(INPUT_DIM).astype(np.float64) + + for _ in range(n_steps): + action = networks.get_allocation(state) + next_state, reward = _env_step(state, action) + networks.replay_buffer.add(state, action, reward, next_state) + state = next_state + + # Buffer is capped at capacity. + assert len(networks.replay_buffer) == CAPACITY + + states, actions, rewards, next_states = networks.replay_buffer.sample(CAPACITY) + critic_loss = networks.update_critic(states, actions, rewards, next_states) + actor_loss = networks.update_actor(states) + networks._soft_update_targets() + + assert math.isfinite(critic_loss) + assert math.isfinite(actor_loss) + + +def test_overflow_exercised_in_training(networks: ActorCriticNetworks) -> None: + """The buffer actually overwrote transitions during the training loop.""" + n_steps = 16 + state = np.random.default_rng(1).standard_normal(INPUT_DIM).astype(np.float64) + for _ in range(n_steps): + action = networks.get_allocation(state) + next_state, reward = _env_step(state, action) + networks.replay_buffer.add(state, action, reward, next_state) + state = next_state + + assert len(networks.replay_buffer) == CAPACITY + # The overflow counter (added with the throttled DEBUG logging) should + # reflect the number of evictions. Guard with hasattr so this test also + # passes if the counter is absent. + if hasattr(networks.replay_buffer, "_overwrite_count"): + assert networks.replay_buffer._overwrite_count == n_steps - CAPACITY + + +def test_training_step_reduces_critic_loss(networks: ActorCriticNetworks) -> None: + """Repeated critic updates on a fixed batch stay finite and do not explode.""" + rng = np.random.default_rng(2) + states = rng.standard_normal((CAPACITY, INPUT_DIM)).astype(np.float64) + actions = np.tile( + np.linspace(0.0, 1.0, NUM_ASSETS), (CAPACITY, 1) + ).astype(np.float64) + rewards = rng.standard_normal(CAPACITY).astype(np.float64) + next_states = rng.standard_normal((CAPACITY, INPUT_DIM)).astype(np.float64) + + loss_before = networks.update_critic(states, actions, rewards, next_states) + for _ in range(20): + loss_after = networks.update_critic(states, actions, rewards, next_states) + + assert math.isfinite(loss_before) + assert math.isfinite(loss_after) + # Soft check: the loss must not explode (DDPG is stochastic, so no strict + # monotonic decrease is asserted). + assert loss_after < 10.0 * max(loss_before, 1e-6) diff --git a/tests/test_replay_buffer.py b/tests/test_replay_buffer.py index f6bff43..5acf278 100644 --- a/tests/test_replay_buffer.py +++ b/tests/test_replay_buffer.py @@ -208,14 +208,14 @@ def test_sample_uses_rng_choice_not_np_random(self) -> None: class TestReplayBufferDebugLogging: - """Tests for TICKET-031: DEBUG logging when buffer is full.""" + """Tests for TICKET-031: counter-based DEBUG logging on overflow.""" def test_debug_log_when_overwriting(self, caplog) -> None: - """A DEBUG message should be emitted when the buffer is full.""" + """A DEBUG message is emitted when the counter hits the interval.""" import logging caplog.set_level(logging.DEBUG) - buf = ReplayBuffer(capacity=3) + buf = ReplayBuffer(capacity=3, log_interval=1) for i in range(3): buf.add( np.array([float(i)]), @@ -223,25 +223,61 @@ def test_debug_log_when_overwriting(self, caplog) -> None: float(i), np.array([float(i + 1)]), ) - # Buffer is now full; next add should trigger debug log + # Buffer is now full; next add is the first overwrite buf.add( np.array([99.0]), np.array([99.0]), 99.0, np.array([100.0]), ) + assert buf._overwrite_count == 1 debug_messages = [ - r for r in caplog.records if r.levelno == logging.DEBUG + r + for r in caplog.records + if r.levelno == logging.DEBUG and "total overwrites" in r.message ] - assert len(debug_messages) >= 1 - assert "overwriting oldest entry" in debug_messages[-1].message + assert len(debug_messages) == 1 + + def test_counter_increments_on_every_overwrite(self) -> None: + """The overwrite counter tracks every eviction, not just logged ones.""" + buf = ReplayBuffer(capacity=3, log_interval=1000) + for i in range(5): + buf.add( + np.array([float(i)]), + np.array([float(i)]), + float(i), + np.array([float(i + 1)]), + ) + # 5 adds into a capacity-3 buffer -> 2 overwrites + assert buf._overwrite_count == 2 + + def test_no_log_below_interval(self, caplog) -> None: + """No DEBUG overflow log is emitted below the configured interval.""" + import logging + + caplog.set_level(logging.DEBUG) + buf = ReplayBuffer(capacity=3) # default log_interval=1000 + for i in range(5): + buf.add( + np.array([float(i)]), + np.array([float(i)]), + float(i), + np.array([float(i + 1)]), + ) + assert buf._overwrite_count == 2 + debug_messages = [ + r + for r in caplog.records + if r.levelno == logging.DEBUG and "total overwrites" in r.message + ] + assert len(debug_messages) == 0 def test_no_debug_log_before_full(self, caplog) -> None: """No DEBUG overwrite message when buffer is not yet full.""" import logging caplog.set_level(logging.DEBUG) - buf = ReplayBuffer(capacity=10) + buf = ReplayBuffer(capacity=10, log_interval=1) for i in range(5): buf.add( np.array([float(i)]), @@ -249,10 +285,15 @@ def test_no_debug_log_before_full(self, caplog) -> None: float(i), np.array([float(i + 1)]), ) + assert buf._overwrite_count == 0 debug_messages = [ r for r in caplog.records - if r.levelno == logging.DEBUG - and "overwriting" in r.message + if r.levelno == logging.DEBUG and "total overwrites" in r.message ] assert len(debug_messages) == 0 + + def test_invalid_log_interval_raises(self) -> None: + """log_interval < 1 is rejected at construction.""" + with pytest.raises(ValueError): + ReplayBuffer(capacity=3, log_interval=0) diff --git a/tickets/TICKET-050.md b/tickets/TICKET-050.md new file mode 100644 index 0000000..2796522 --- /dev/null +++ b/tickets/TICKET-050.md @@ -0,0 +1,93 @@ +# TICKET-050: ReplayBuffer.add() should use counter-based DEBUG logging on overflow + +- **GitHub issue:** #98 (open) +- **Original ticket:** TICKET-031 +- **Target module:** `alloc/models/networks.py` — `ReplayBuffer` +- **Status:** open + +## Evidence + +`alloc/models/networks.py` lines 94-99 — `ReplayBuffer.add()` currently logs +**per-overwrite** at DEBUG: + + if len(self._buffer) == self._capacity: + logger.debug( + "ReplayBuffer full (capacity=%d), overwriting oldest entry", + self._capacity, + ) + self._buffer.append((state, action, float(reward), next_state)) + +- There is **no `_overwrite_count` counter** anywhere in the class. +- `ReplayBuffer.__init__` (lines 50-72) has no counter field and no throttle + parameter. +- Issue #98 spec: use a **counter-based** approach — increment a counter on each + overwrite and log **every 1000th** overwrite at DEBUG. Log format: + `"ReplayBuffer full (capacity=%d), %d total overwrites"`. + +The current implementation only avoids *INFO*-level spam. It does **not** avoid +*DEBUG*-level spam: once the buffer is full, every `add()` emits one DEBUG line. + +## Impact + +- **DEBUG spam during training.** The default `ActorCriticNetworks` uses + `buffer_capacity=1_000_000` (networks.py:283). A training run pushes millions + of transitions; after warmup the buffer is full and every subsequent `add()` + logs. At DEBUG that is ~1M log lines — the exact spam issue #98 targets. +- **Existing test will break.** `tests/test_replay_buffer.py:213` + (`test_debug_log_when_overwriting`) asserts the per-overwrite message + `"overwriting oldest entry"` (line 237). Changing the message text and the + emission cadence breaks this test. It must be updated in the same change. + +## Suggestion + +Make the throttle counter-based and configurable so it is testable. + +### Implementation plan + +1. **`ReplayBuffer.__init__`** (networks.py:50-72): + - Add a `log_interval: int = 1000` parameter (default 1000 for production). + - Add `self._overwrite_count: int = 0` and `self._log_interval: int = log_interval` + (place after line 71, before the `logger.info` at line 72). + - Validate `log_interval >= 1` (raise `ValueError` otherwise), mirroring the + existing `capacity <= 0` guard at line 63. + +2. **`ReplayBuffer.add()`** (networks.py:74-99): replace the per-overwrite log + with a counter + throttle: + + if len(self._buffer) == self._capacity: + self._overwrite_count += 1 + if self._overwrite_count % self._log_interval == 0: + logger.debug( + "ReplayBuffer full (capacity=%d), %d total overwrites", + self._capacity, + self._overwrite_count, + ) + self._buffer.append((state, action, float(reward), next_state)) + +3. **Update `tests/test_replay_buffer.py::TestReplayBufferDebugLogging`** + (lines 210-251): + - Rewrite `test_debug_log_when_overwriting` (line 213): construct + `ReplayBuffer(capacity=3, log_interval=1)`, add 4 transitions (1 overwrite), + assert `buf._overwrite_count == 1` and that a DEBUG record containing + `"total overwrites"` was emitted (via `caplog.set_level(logging.DEBUG)`). + - Add `test_counter_increments_on_every_overwrite`: `capacity=3`, add 5 + transitions, assert `buf._overwrite_count == 2`. + - Add `test_no_log_below_interval`: `capacity=3`, default `log_interval=1000`, + add 5 transitions (2 overwrites), assert **no** DEBUG record containing + `"total overwrites"` was emitted (2 < 1000). + - Keep `test_no_debug_log_before_full` (line 239) — still valid. + +### Spec tension (flagged) + +Issue #98's test description ("add 5 transitions, assert `_overwrite_count == 2`, +verify a DEBUG log was emitted") is internally inconsistent with the every-1000th +rule: 2 overwrites < 1000, so no log is emitted. The `log_interval` parameter +resolves this by making the throttle configurable for tests while defaulting to +1000 in production. + +## Verification + +- `pytest tests/test_replay_buffer.py -xvs` — all pass, including the 3 updated/new + overflow-logging tests. +- `ruff check alloc/models/networks.py` — clean. +- `mypy alloc/models/networks.py --ignore-missing-imports` — clean. diff --git a/tickets/TICKET-051.md b/tickets/TICKET-051.md new file mode 100644 index 0000000..26b54d2 --- /dev/null +++ b/tickets/TICKET-051.md @@ -0,0 +1,119 @@ +# TICKET-051: Add a true closed-loop DDPG training-step integration test + +- **GitHub issue:** #100 (open) +- **Original ticket:** TICKET-033 +- **Target:** `tests/` (new file `tests/test_ddpg_integration.py`) +- **Status:** open + +## Evidence + +Issue #100's body states: "There is **no integration test** that exercises the +complete DDPG training loop." That claim is **stale** — a `TestDDPGTrainingStep` +class already exists at `tests/test_actor_critic.py:252` (added in commit +`987b236`, PR #52), with `test_full_training_step` (line 270) exercising +`_sample_action` → `replay_buffer.add` → `replay_buffer.sample` → +`update_critic` → `update_actor` → `_soft_update_targets`. + +However, the existing test is **synthetic, not a real training step**: + +1. **Uses the private `_sample_action`** (test_actor_critic.py:283, 342, 376) + rather than the public inference entry point `get_allocation` + (networks.py:475). A real training loop calls the public API. +2. **`next_states` are independent random draws** (line 276: + `np.random.randn(batch_size, 10)`), not derived from the action. The DDPG + Bellman target in `update_critic` (networks.py:565-627) assumes + `next_state` is the environment's response to `action`. Independent + `next_states` do not model the closed-loop dynamics. +3. **The buffer is never filled.** `buffer_capacity=1000` (line 264) but only + `batch_size=8` transitions are added (line 270). The overflow path + (networks.py:94-99) is never exercised in the integration test. +4. **No reward model.** Rewards are `np.random.randn` (line 275) with no + relationship to the allocation action, so the test cannot verify that the + critic learns anything meaningful — only that it does not produce NaN. + + +## Impact + +- **False confidence.** The existing test passes, so issue #100 appears closed, + but it does not validate the real training loop: public action sampling, + closed-loop state transitions, buffer overflow, or reward-driven learning. +- **Overflow path untested end-to-end.** The `ReplayBuffer` overflow logging + (TICKET-050 / issue #98) and the deque eviction are only covered in isolation + in `tests/test_replay_buffer.py`, never through the full + `ActorCriticNetworks` training path. +- **Public API untested in a training context.** `get_allocation` is tested in + isolation (`TestGetAllocation`, test_actor_critic.py:124) but never as the + action source feeding a training step. + +## Suggestion + +Add a dedicated integration test file `tests/test_ddpg_integration.py` that +exercises a **true closed-loop** DDPG training step. Do **not** modify the +existing `TestDDPGTrainingStep` (it remains valid as a synthetic smoke test); +add the new file so both perspectives are covered. + +### Implementation plan + +1. **New file `tests/test_ddpg_integration.py`** with a small deterministic + environment closure so `next_state` is a function of `(state, action)`: + + def env_step(state, action): + # Deterministic toy dynamics: next_state depends on action. + # e.g. next_state = state * 0.9 + action * 0.1 (broadcast over assets) + ... + reward = float(np.dot(action, state)) # reward tied to allocation + return next_state, reward + +2. **`test_closed_loop_training_step`** — the core integration test: + - Build `ActorCriticNetworks(input_dim=10, num_assets=5, seed=42, + min_cash_allocation=0.05, buffer_capacity=8)` (small capacity so overflow + is reachable). + - Loop over `N=16` steps (N > capacity so the buffer overflows): + - `action = networks.get_allocation(state)` (public API, greedy). + - `next_state, reward = env_step(state, action)` (closed-loop). + - `networks.replay_buffer.add(state, action, reward, next_state)`. + - `state = next_state`. + - Assert `len(networks.replay_buffer) == 8` (capped at capacity). + - `states, actions, rewards, next_states = networks.replay_buffer.sample(8)`. + - `critic_loss = networks.update_critic(states, actions, rewards, next_states)`. + - `actor_loss = networks.update_actor(states)`. + - `networks._soft_update_targets()`. + - Assert both losses are finite floats. + +3. **`test_overflow_exercised_in_training`** — verify the buffer actually + overwrote during the loop: + - Same setup; after the loop assert the buffer is full and that the oldest + transitions were evicted (e.g. rewards reflect the most recent 8 steps, + not the first 8). + - Optionally assert `networks.replay_buffer._overwrite_count == N - capacity` + (depends on TICKET-050 landing; guard with `hasattr`). + +4. **`test_training_step_reduces_critic_loss`** (soft check): + - Run several critic updates on a fixed batch; assert the loss is finite and + does not explode (e.g. `loss_after < 10 * loss_before`), not a strict + monotonic decrease (DDPG is stochastic). + + +## Verification + +- `pytest tests/test_ddpg_integration.py -xvs` — all new tests pass. +- `pytest tests/test_actor_critic.py -xvs` — existing `TestDDPGTrainingStep` + still passes (unchanged). +- `pytest tests/test_replay_buffer.py -xvs` — no regressions. +- `ruff check tests/test_ddpg_integration.py` — clean. +- `mypy tests/test_ddpg_integration.py --ignore-missing-imports` — clean. + +## Notes + +- **Semantics reference:** DDPG semantics were read from + `~/Research/new-trader/trader/models/networks.py` for understanding only + (Bellman target, soft target update, actor gradient ascent on Q). Nothing was + copied; the test targets the `alloc` implementation's actual public API + (`get_allocation`, `update_critic`, `update_actor`, `_soft_update_targets`). +- **Dependency on TICKET-050:** `test_overflow_exercised_in_training` references + `_overwrite_count`, which only exists after TICKET-050 (issue #98) lands. The + test must guard with `hasattr(networks.replay_buffer, "_overwrite_count")` so + it passes independently, or be added in the same PR as TICKET-050. +- **Keep the synthetic test:** `TestDDPGTrainingStep` (test_actor_critic.py:252) + stays as a fast smoke test. The new file adds the closed-loop + overflow + coverage that issue #100 actually requires.