From a5c3d73d9ef69ed1d9be256bb5684464f8baed5d Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Thu, 23 Apr 2026 08:44:05 +0200 Subject: [PATCH] cleanup: declarative rewrites for ml-tests and trading-engine TODOs - ml/tests/dqn_training_pipeline_test.rs: the inline TODO speculated about a future `load_checkpoint` hook; loader round-trip coverage already lives in dqn_checkpoint_tests. Reword to point there. - ml/tests/ppo_lstm_training_loop_tests.rs: the assertion on `hidden_state_manager.is_some()` is the public-surface proxy for "LSTM path active"; deeper introspection isn't exposed. Say so. - ml/tests/ppo_recurrent_integration_tests.rs: the test is already `#[ignore]`d; rewrite the inline TODO as a description of the missing `from_varbuilder` constructors on LSTMPolicyNetwork / LSTMValueNetwork. - risk/risk_engine.rs: VarEngine receives a default asset-class config because the schema-to-config conversion is not wired. Reword the TODO to describe that plainly. - trading_engine/types/errors.rs: `common::ConversionError` does not exist; keep ConversionError local and drop the aspirational re-export comment. - trading_engine/tests/audit_persistence_tests.rs: describe why the query assertion only checks the Ok shape (row-to-event mapping not wired) rather than pointing at a nonexistent line number. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/ml/tests/dqn_training_pipeline_test.rs | 5 +++-- crates/ml/tests/ppo_lstm_training_loop_tests.rs | 5 +++-- crates/ml/tests/ppo_recurrent_integration_tests.rs | 7 ++++--- crates/risk/src/risk_engine.rs | 8 +++++--- crates/trading_engine/src/types/errors.rs | 8 ++++---- crates/trading_engine/tests/audit_persistence_tests.rs | 7 +++++-- 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/crates/ml/tests/dqn_training_pipeline_test.rs b/crates/ml/tests/dqn_training_pipeline_test.rs index b59a21faf..ef9cce8fc 100644 --- a/crates/ml/tests/dqn_training_pipeline_test.rs +++ b/crates/ml/tests/dqn_training_pipeline_test.rs @@ -398,8 +398,9 @@ async fn test_dqn_checkpoint_save_load() -> Result<()> { assert!(checkpoint_size > 1024, "Checkpoint should be >1KB"); - // TODO: Once we have a load_checkpoint method, test loading here - // For now, just verify the file is valid SafeTensors format + // Verify the checkpoint bytes round-trip back from disk. A dedicated + // `load_checkpoint` entry point is not part of this end-to-end test; + // roundtrip coverage for the loader lives in `dqn_checkpoint_tests`. let checkpoint_data = std::fs::read(&saved_checkpoint_path)?; assert!( checkpoint_data.len() == checkpoint_size as usize, diff --git a/crates/ml/tests/ppo_lstm_training_loop_tests.rs b/crates/ml/tests/ppo_lstm_training_loop_tests.rs index 4bc5386c6..43a125db5 100644 --- a/crates/ml/tests/ppo_lstm_training_loop_tests.rs +++ b/crates/ml/tests/ppo_lstm_training_loop_tests.rs @@ -192,8 +192,9 @@ fn test_ppo_training_with_lstm_enabled() { "Hidden state manager should be initialized when use_lstm=true" ); - // TODO: Add verification that LSTM networks are actually being used - // This requires checking network types or tracking LSTM state updates + // The `hidden_state_manager.is_some()` check above is the observable + // proxy for "LSTM path is active"; deeper introspection of the + // underlying network types is not surfaced through the public API. // Create dummy trajectory batch let mut batch = create_dummy_trajectory_batch(10, config.state_dim); diff --git a/crates/ml/tests/ppo_recurrent_integration_tests.rs b/crates/ml/tests/ppo_recurrent_integration_tests.rs index ecad41e42..eb46b5e2a 100644 --- a/crates/ml/tests/ppo_recurrent_integration_tests.rs +++ b/crates/ml/tests/ppo_recurrent_integration_tests.rs @@ -391,9 +391,10 @@ fn test_recurrent_vs_feedforward_ppo() { #[test] #[ignore = "LSTM checkpoint loading not yet implemented (requires from_varbuilder methods in lstm_networks.rs)"] fn test_recurrent_ppo_checkpointing() { - // Test that checkpoints can be saved and loaded with LSTM configuration - // TODO: Implement from_varbuilder methods in LSTMPolicyNetwork and LSTMValueNetwork - // to enable loading LSTM checkpoints from safetensors files + // Test that checkpoints can be saved and loaded with LSTM configuration. + // `LSTMPolicyNetwork` / `LSTMValueNetwork` currently lack `from_varbuilder` + // constructors, so safetensors-based checkpoint loading is not wired; the + // test is left under #[ignore] until those constructors exist. let config = PPOConfig { state_dim: 16, num_actions: 63, diff --git a/crates/risk/src/risk_engine.rs b/crates/risk/src/risk_engine.rs index 9ff0fc9f9..9c501ab17 100644 --- a/crates/risk/src/risk_engine.rs +++ b/crates/risk/src/risk_engine.rs @@ -890,9 +890,11 @@ impl RiskEngine { // Initialize metrics collector (fix: provide max_samples parameter) let metrics = Arc::new(RiskMetricsCollector::new(10000)); - // Initialize VarEngine with the var_config and asset classification - // Convert AssetClassificationSchema to AssetClassificationConfig - let asset_config = AssetClassificationConfig::default(); // TODO: proper conversion + // Initialize VarEngine with the var_config. The VaR engine currently + // uses `AssetClassificationConfig::default()` — a schema-to-config + // conversion from `config.asset_classification` is not wired, so the + // VaR side sees default asset buckets regardless of user schema. + let asset_config = AssetClassificationConfig::default(); let var_engine = Arc::new(VarEngine::new(config.var_config.clone(), asset_config)); // Initialize circuit breaker if enabled (safe configuration) diff --git a/crates/trading_engine/src/types/errors.rs b/crates/trading_engine/src/types/errors.rs index a8d6baa47..bb854519a 100644 --- a/crates/trading_engine/src/types/errors.rs +++ b/crates/trading_engine/src/types/errors.rs @@ -14,10 +14,10 @@ use serde::{Deserialize, Serialize}; use std::fmt; use thiserror::Error; -// Re-export common error types for convenience -// TODO: Import these from common crate once they exist there -// pub use common::ConversionError; -// Note: ProtocolError and SymbolError are defined locally in this module +// `ConversionError` is defined locally in this module. The `common` +// crate does not currently export a parallel error type, so nothing +// is re-exported here. `ProtocolError` and `SymbolError` also live +// in this module. /// Conversion error types used by trading engine #[derive(Error, Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/crates/trading_engine/tests/audit_persistence_tests.rs b/crates/trading_engine/tests/audit_persistence_tests.rs index 8d6383260..b87d7226e 100644 --- a/crates/trading_engine/tests/audit_persistence_tests.rs +++ b/crates/trading_engine/tests/audit_persistence_tests.rs @@ -716,9 +716,12 @@ mod query_engine_tests { }; let result = engine.query(query).await; - // Note: Query will succeed but return empty results until row-to-event mapping is implemented (TODO line 1109) + // The audit-persistence layer accepts the query and returns an + // empty result set because row-to-event mapping is not wired in + // the engine's query path yet. Assert only that the RPC + // succeeds — shape assertions come online with the mapping. assert!(result.is_ok(), "Query failed: {:?}", result.err()); - println!("✅ Query by time range successful (implementation pending row mapping)"); + println!("✅ Query by time range returned Ok (row mapping not yet wired)"); } #[tokio::test]