majiayu000 Architecture Reviewer
Review code against PRISM's target architecture patterns to ensure consistency with the refactoring design.
Purpose
This skill validates that new and refactored code follows PRISM's architectural patterns, specifically the Template Method pattern for Runners and the Strategy pattern for Trainers.
When to Use
Use this skill when:
- Creating new Runner classes (PRISMRunner, MoPIERunner)
- Creating new Trainer classes (ProgressiveTrainer, EpochalTrainer)
- Refactoring existing procedural code to object-oriented patterns
- Verifying Template Method pattern implementation
- Checking Strategy pattern usage
- Reviewing pull requests that modify core architecture
Target Architecture Patterns
Pattern 1: AbstractRunner (Template Method)
The Template Method pattern defines a skeleton workflow with customizable hooks:
from abc import ABC, abstractmethod
from dataclasses import dataclass
from pathlib import Path
from typing import Optional
import torch
from torch.utils.tensorboard import SummaryWriter
@dataclass
class ExperimentResult:
"""Result of an experiment run."""
ssims: list[float]
psnrs: list[float]
rmses: list[float]
final_reconstruction: torch.Tensor
log_dir: Path
elapsed_time: float
class AbstractRunner(ABC):
"""Abstract base class for experiment runners.
Implements Template Method pattern: run() defines the workflow,
subclasses implement hook methods.
"""
def __init__(self, args):
self.args = args
self.config = None
self.device = None
self.log_dir = None
self.writer: Optional[SummaryWriter] = None
def run(self) -> ExperimentResult:
"""Template method - DO NOT OVERRIDE in subclasses."""
self.setup() # Hook 1: Environment setup
self.load_data() # Hook 2: Load image/pattern
self.create_components() # Hook 3: Create model/trainer
result = self.run_experiment() # Hook 4: Run training
self.save_results(result) # Hook 5: Save outputs
self.cleanup() # Hook 6: Resource cleanup
return result
@abstractmethod
def setup(self) -> None:
"""Hook 1: Set up device, logging, directories."""
...
@abstractmethod
def load_data(self) -> None:
"""Hook 2: Load image and generate pattern."""
...
@abstractmethod
def create_components(self) -> None:
"""Hook 3: Create model, telescope, trainer."""
...
@abstractmethod
def run_experiment(self) -> ExperimentResult:
"""Hook 4: Run training and return results."""
...
@abstractmethod
def save_results(self, result: ExperimentResult) -> None:
"""Hook 5: Save checkpoint and visualizations."""
...
def cleanup(self) -> None:
"""Hook 6: Cleanup (default implementation)."""
if self.writer:
self.writer.close()
Pattern 2: AbstractTrainer (Strategy Pattern)
The Strategy pattern allows interchangeable training algorithms:
from abc import ABC, abstractmethod
from dataclasses import dataclass
import torch
from torch import nn, Tensor
@dataclass
class TrainingResult:
"""Final training result."""
ssims: list[float]
psnrs: list[float]
rmses: list[float]
final_reconstruction: Tensor
total_time: float
converged: bool
class AbstractTrainer(ABC):
"""Abstract base class for training strategies."""
def __init__(
self,
model: nn.Module,
device: torch.device,
config: 'TrainingConfig'
):
self.model = model
self.device = device
self.config = config
@abstractmethod
def train(self) -> TrainingResult:
"""Main training loop - must be implemented."""
...
@abstractmethod
def train_step(self, batch_idx: int) -> dict:
"""Single training step - must be implemented."""
...
def compute_metrics(self) -> tuple[float, float, float]:
"""Compute RMSE, SSIM, PSNR (can be overridden)."""
return self.model.errors()
def should_stop(self) -> bool:
"""Early stopping check (can be overridden)."""
return False
def save_checkpoint(self, path: Path) -> None:
"""Save checkpoint (can be overridden)."""
torch.save({
'model_state': self.model.state_dict(),
'config': self.config,
}, path)
Pattern 3: Concrete Runner Example
class PRISMRunner(AbstractRunner):
"""Runner for PRISM algorithm."""
def setup(self) -> None:
self.device = setup_device(self.args)
self.config = load_config(self.args)
self.log_dir = Path(self.args.log_dir) / self.args.name
self.writer = SummaryWriter(self.log_dir)
def load_data(self) -> None:
self.image = load_image(self.args.obj_name).to(self.device)
self.centers = generate_pattern(self.args, self.config)
def create_components(self) -> None:
self.telescope = Telescope(...).to(self.device)
self.model = ProgressiveDecoder(...).to(self.device)
self.trainer = ProgressiveTrainer(self.model, ...)
def run_experiment(self) -> ExperimentResult:
result = self.trainer.train()
return ExperimentResult(
ssims=result.ssims,
psnrs=result.psnrs,
rmses=result.rmses,
final_reconstruction=result.final_reconstruction,
log_dir=self.log_dir,
elapsed_time=result.total_time
)
def save_results(self, result: ExperimentResult) -> None:
save_checkpoint(self.model, self.log_dir / 'checkpoint.pt')
create_visualization(result, self.log_dir)
Architecture Review Checklist
For AbstractRunner Subclasses
- Inheritance: Class inherits from
AbstractRunner - Constructor: Calls
super().__init__(args)first - Template Method: Does NOT override
run() - Hook Implementations:
-
setup(): Sets device, config, log_dir, writer -
load_data(): Loads image and pattern only -
create_components(): Creates model, telescope, trainer -
run_experiment(): Calls trainer.train(), returns ExperimentResult -
save_results(): Saves checkpoint and visualizations
-
- Separation of Concerns: Each hook does one thing
- Type Hints: All methods have proper annotations
- Docstrings: All methods documented
For AbstractTrainer Subclasses
- Inheritance: Class inherits from
AbstractTrainer - Constructor: Calls
super().__init__(model, device, config) - Required Methods:
-
train(): Returns TrainingResult -
train_step(): Single step logic
-
- Metrics: Uses SSIM, PSNR, RMSE consistently
- Checkpointing: Supports save_checkpoint()
- Type Hints: All methods properly typed
General Code Quality
- Naming:
- Runners end with "Runner" (PRISMRunner, MoPIERunner)
- Trainers end with "Trainer" (ProgressiveTrainer, EpochalTrainer)
- Methods use snake_case
- Classes use PascalCase
- No God Functions: Methods < 50 lines
- No Hardcoded Values: Use config or parameters
- Testability: Components can be instantiated independently
Anti-Patterns to Avoid
Anti-Pattern 1: Overriding Template Method
# BAD - Don't override run()
class MyRunner(AbstractRunner):
def run(self) -> ExperimentResult: # DON'T DO THIS
# Custom implementation
...
Anti-Pattern 2: Mixing Concerns
# BAD - setup() doing too much
def setup(self) -> None:
self.device = setup_device(self.args)
self.image = load_image(...) # This belongs in load_data()!
self.model = create_model(...) # This belongs in create_components()!
Anti-Pattern 3: Trainer Doing Visualization
# BAD - Trainer shouldn't handle visualization
class MyTrainer(AbstractTrainer):
def train(self) -> TrainingResult:
result = self._train_loop()
self._create_plots() # This belongs in Runner.save_results()!
return result
Anti-Pattern 4: No Abstraction
# BAD - Doesn't use abstract base
class MoPIERunner: # Should inherit from AbstractRunner!
def run(self):
# Duplicate implementation
...
Anti-Pattern 5: Hardcoded Paths
# BAD
def save_results(self, result):
torch.save(model, '/tmp/checkpoint.pth') # Use self.log_dir!
Review Workflow
Step 1: Check Inheritance
# Verify proper inheritance
assert issubclass(MyRunner, AbstractRunner)
assert issubclass(MyTrainer, AbstractTrainer)
Step 2: Check Hook Implementations
# Verify all abstract methods are implemented
required_runner_methods = ['setup', 'load_data', 'create_components',
'run_experiment', 'save_results']
for method in required_runner_methods:
assert hasattr(MyRunner, method)
assert callable(getattr(MyRunner, method))
Step 3: Check Method Signatures
# Verify return types
import inspect
sig = inspect.signature(MyRunner.run_experiment)
assert sig.return_annotation == ExperimentResult
Step 4: Check for Anti-Patterns
- Search for
def run(self)in Runner subclasses (shouldn't exist) - Check that hooks don't overlap in functionality
- Verify no visualization code in Trainers
Example Review Comments
Good Implementation
✅ Excellent! PRISMRunner correctly implements AbstractRunner pattern. All hooks are focused and follow single responsibility. Type hints and docstrings are complete.
Needs Improvement
❌ Issues found in MoPIERunner:
- Doesn't inherit from AbstractRunner
- setup() loads data (should be in load_data())
- Missing type hints on run_experiment()
Recommendation: Refactor to follow AbstractRunner pattern.
Quick Reference
Runner Template
AbstractRunner.run():
1. setup() → Device, config, logging
2. load_data() → Image, pattern
3. create_components() → Model, telescope, trainer
4. run_experiment() → Training loop
5. save_results() → Checkpoint, visualization
6. cleanup() → Resource cleanup
Trainer Template
AbstractTrainer.train():
Loop:
1. train_step() → Process batch
2. compute_metrics() → RMSE, SSIM, PSNR
3. should_stop() → Early stopping
Return TrainingResult
Related Skills
- type-hint-adder: Add type hints to Runner/Trainer classes
- unit-test-generator: Create architecture compliance tests
- docstring-formatter: Document patterns in code
Checklist Before Merge
- All checklist items verified
- No anti-patterns detected
- Follows naming conventions
- Properly typed and documented
- Consistent with existing patterns
- Tests pass