Feat/memory train

[chenjw]

Description

Related Issue

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactoring (no functional changes)
• Performance improvement
• Test update

Changes Made

Testing

• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have tested this on the following platforms:
  • Linux
  • macOS
  • Windows

Checklist

• My code follows the project's coding style
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🏅 Score: 70

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add streaming memory updater and compressor V3 Relevant files: openviking/session/memory/streaming_memory_updater.py openviking/session/compressor_v3.py tests/session/memory/test_streaming_memory_updater.py tests/session/test_compressor_v3.py tests/integration/test_compressor_v3_case_extraction.py Sub-PR theme: Add training framework and patch merge context provider Relevant files: openviking/session/train/**/*.py tests/session/train/**/*.py tests/session/memory/test_patch_merge_context_provider.py

⚡ Recommended focus areas for review Missing VLM parameter for training components The ExperienceGradientEstimator and PatchMergePolicyOptimizer are initialized without the required 'vlm' parameter, which will prevent them from functioning correctly (as seen in the test file where 'vlm' is explicitly provided). gradient_estimator=ExperienceGradientEstimator( viking_fs=viking_fs, ), policy_optimizer=PatchMergePolicyOptimizer( viking_fs=viking_fs, memory_type="experiences", ), Silent exception handling without logging The MemoryFileUtils.write call is wrapped in a broad exception handler that falls back to operation_after_content without logging the failure, which will make debugging issues difficult. ) except Exception: return operation_after_content(op) Potential incorrect VLM instance retrieval The code calls get_vlm_instance() on get_openviking_config().vlm, but test files use get_openviking_config().vlm directly as the VLM instance. This could cause an AttributeError if config.vlm is already the instance. vlm = get_openviking_config().vlm.get_vlm_instance()

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add missing VLM instance to policy gradient/optimizer The ExperienceGradientEstimator and PatchMergePolicyOptimizer are likely missing a required VLM instance (as seen in the test file). Retrieve the VLM from the openviking config and pass it to both initializers. openviking/session/compressor_v3.py [389-395] +config = get_openviking_config() +vlm = config.vlm.get_vlm_instance() gradient_estimator=ExperienceGradientEstimator( viking_fs=viking_fs, + vlm=vlm, ), policy_optimizer=PatchMergePolicyOptimizer( viking_fs=viking_fs, + vlm=vlm, memory_type="experiences", ), Suggestion importance[1-10]: 7 __ Why: The test files show that ExperienceGradientEstimator and PatchMergePolicyOptimizer require a vlm parameter for proper operation, and the codebase already uses get_openviking_config().vlm.get_vlm_instance() to retrieve the VLM elsewhere, making this a likely missing initialization issue.	Medium
General	Remove unnecessary field() usage from non-dataclass The SessionCompressorV3 class is using field() for class attributes but is not decorated with @dataclass. This is misleading because field() only works with dataclasses. Remove the field() usage since the init method properly initializes these attributes. openviking/session/compressor_v3.py [74-80] class SessionCompressorV3: """Session compressor with lock-free patch-merge user memory extraction.""" rollout_analyzer: TrajectoryRolloutAnalyzer | Any - streaming_trainer_config: StreamingPolicyTrainerConfig = field( - default_factory=StreamingPolicyTrainerConfig - ) - streaming_memory_updater_config: StreamingMemoryUpdaterConfig = field( - default_factory=StreamingMemoryUpdaterConfig - ) + streaming_trainer_config: StreamingPolicyTrainerConfig + streaming_memory_updater_config: StreamingMemoryUpdaterConfig Suggestion importance[1-10]: 5 __ Why: The SessionCompressorV3 class is not a dataclass, so using field() for class attributes is misleading and unnecessary, as __init__ properly initializes them. Removing the field()` usage improves code clarity.	Low