Build initial prediction engine service#559
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new "predictionengine" application with its own Dockerfile, dependencies, and a complete time series prediction pipeline, including custom deep learning modules and comprehensive tests. Several configuration and Docker files are updated for Python 3.12 compatibility, improved coverage handling, environment variable defaults, and Docker Compose service refinement. Dependency and workspace management are revised across the project. Minor fixes and improvements are made in environment variable handling, error checking, and test workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI App
participant DataManager
participant Model
Client->>FastAPI App: POST /create-predictions
FastAPI App->>DataManager: GET /historical-equity-bars
DataManager-->>FastAPI App: Arrow IPC data
FastAPI App->>Model: load_or_initialize_model(data)
loop For each ticker
FastAPI App->>Model: predict(ticker, features)
Model-->>FastAPI App: quantile predictions
end
FastAPI App-->>Client: PredictionResponse
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (16)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the initial prediction engine service, including data preprocessing, model architecture, and an API layer for serving predictions.
- Implements core components: data ingestion (
DataSet), model pipeline (MiniatureTemporalFusionTransformerwith LSTM, gated residual blocks, multi-head self-attention), and post-processing of outputs. - Defines FastAPI endpoints for health checks and creating predictions, including error handling.
- Configures project metadata, dependencies, and CI tasks for coverage reporting.
Reviewed Changes
Copilot reviewed 27 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| application/predictionengine/src/predictionengine/post_processor.py | Decodes encoded tickers and rescales model outputs to percentiles |
| application/predictionengine/src/predictionengine/multi_head_self_attention.py | Implements multi-head self-attention block |
| application/predictionengine/src/predictionengine/models.py | Defines Pydantic schema for prediction responses |
| application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py | Assembles full model pipeline and API-facing methods |
| application/predictionengine/src/predictionengine/loss_function.py | Implements quantile loss function |
| application/predictionengine/src/predictionengine/long_short_term_memory.py | Provides a stacked LSTM implementation |
| application/predictionengine/src/predictionengine/gated_residual_network.py | Defines a gated residual network block |
| application/predictionengine/src/predictionengine/dataset.py | Loads, encodes, scales, and batches time-series data |
| application/predictionengine/src/predictionengine/main.py | FastAPI routes for health and prediction endpoints |
| application/predictionengine/pyproject.toml | Project metadata and dependencies |
| .mise.toml | Adds coverage output directory for tests |
| .github/workflows/test.yaml | Updates coverage report path in CI |
| application/positionmanager/src/positionmanager/main.py | Adds default empty strings for missing env vars |
| application/positionmanager/src/positionmanager/clients.py | Improves cash balance handling and pivot parameters |
| application/datamanager/src/datamanager/config.py | Provides default values for required env vars |
| application/datamanager/pyproject.toml | Adds instrumentator and logging dependencies |
Files not reviewed (4)
- Dockerfile.test: Language not supported
- application/datamanager/Dockerfile: Language not supported
- application/positionmanager/Dockerfile: Language not supported
- application/predictionengine/Dockerfile: Language not supported
Comments suppressed due to low confidence (2)
application/predictionengine/src/predictionengine/multi_head_self_attention.py:13
- Remove the debugging print statement in the constructor to avoid unwanted console output.
print("check:", embedding_size % heads_count)
application/predictionengine/pyproject.toml:14
- Remove the self-dependency on
predictionenginein its ownpyproject.tomlto avoid circular dependency issues.
"predictionengine",
Graphite Automations"Assign author to pull request" took an action on this PR • (05/26/25)1 assignee was added to this PR based on John Forstmeier's automation. |
|
@chrisaddy well fuck I broke the GitHub actions tests again with this PR. I updated the Docker stuff because the volume definitions were messing with dependencies in the local virtual environment but now apparently that borked the workflow. I'll need to look into this more before it gets merged. Update: this has been fixed but I'm addressing feedback still. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (2)
application/predictionengine/tests/test_loss_function.py (1)
51-59:⚠️ Potential issueComplete the test implementation.
The test function is incomplete - it's missing the final assertion after line 58. This could cause the test to not actually verify the tensor type.
def test_quantile_loss_shapes(): for batch_size in [1, 2, 4, 8]: predictions = Tensor(np.random.randn(batch_size, 3)) targets = Tensor(np.random.randn(batch_size, 1)) quantiles = (0.25, 0.5, 0.75) loss = quantile_loss(predictions, targets, quantiles) assert isinstance(loss, Tensor) + assert loss.shape == () or loss.shape == (1,)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (1)
176-191: 🛠️ Refactor suggestionRemove duplicate post-processing logic.
The
forwardmethod already returns the quantile predictions as the third element of the tuple. The post-processing in thepredictmethod is redundant.def predict( self, tickers: Tensor, input: Tensor, ) -> Tuple[np.ndarray, np.ndarray, np.ndarray]: - predictions, _, _ = self.forward(tickers, input) - - percentile_25, percentile_50, percentile_75 = ( - self.post_processor.post_process_predictions( - tickers.numpy(), - predictions.numpy(), - ) - ) - - return percentile_25, percentile_50, percentile_75 + _, _, quantiles = self.forward(tickers, input) + return quantiles
♻️ Duplicate comments (1)
application/positionmanager/src/positionmanager/main.py (1)
135-136: Same configuration masking issue in delete endpoint.The same problem exists here - empty string defaults for API credentials will cause authentication failures at runtime instead of failing fast with a clear configuration error.
🧹 Nitpick comments (19)
application/predictionengine/src/predictionengine/ticker_embedding.py (2)
6-7: Consider adding input validation.While the current implementation is correct, consider adding basic input validation to ensure
ticker_countandembedding_sizeare positive integers.def __init__(self, ticker_count: int, embedding_size: int): + if ticker_count <= 0 or embedding_size <= 0: + raise ValueError("ticker_count and embedding_size must be positive integers") self.embedding = Embedding(ticker_count, embedding_size)
1-11: Consider adding class docstring.Adding a docstring would improve code documentation and help other developers understand the purpose and usage of this embedding layer.
+"""Ticker embedding layer for neural network models.""" from tinygrad.nn import Embedding from tinygrad.tensor import Tensor class TickerEmbedding: + """ + Embedding layer for ticker symbols in financial prediction models. + + Args: + ticker_count: Number of unique ticker symbols + embedding_size: Dimension of the embedding vectors + """ def __init__(self, ticker_count: int, embedding_size: int):application/predictionengine/src/predictionengine/models.py (2)
1-2: Use modern type annotations.Consider using built-in
dictinstead oftyping.Dictfor Python 3.9+ compatibility and modern best practices.-from pydantic import BaseModel -from typing import Dict +from pydantic import BaseModel
5-6: Consider more specific typing and documentation.The nested dictionary structure could benefit from clearer typing and documentation about what the string keys represent.
class PredictionResponse(BaseModel): - predictions: Dict[str, Dict[str, float]] + predictions: dict[str, dict[str, float]] + """ + Predictions mapped by ticker symbol to quantile predictions. + + Format: {ticker: {quantile: prediction_value}} + Example: {"AAPL": {"0.1": 150.5, "0.5": 155.0, "0.9": 160.2}} + """Alternatively, consider using more structured types:
from typing import Annotated from pydantic import BaseModel, Field class PredictionResponse(BaseModel): predictions: Annotated[ dict[str, dict[str, float]], Field(description="Predictions by ticker symbol to quantile predictions") ]application/predictionengine/tests/test_loss_function.py (1)
30-38: Add assertion to verify the actual loss value for perfect prediction.The test only checks that loss is non-negative, but for a perfect prediction (target matches median), the quantile loss should be more predictable. Consider adding a more specific assertion.
def test_quantile_loss_perfect_prediction(): predictions = Tensor([[1.0, 2.0, 3.0]]) targets = Tensor([[2.0]]) # matches median prediction quantiles = (0.25, 0.5, 0.75) loss = quantile_loss(predictions, targets, quantiles) - assert loss.numpy() >= 0.0 + assert loss.numpy() >= 0.0 + # For perfect median prediction, loss should be relatively small + assert loss.numpy() < 1.0 # Add upper bound checkapplication/predictionengine/Dockerfile (3)
9-11: Optimize Docker layer caching by copying requirements first.Consider copying only the pyproject.toml file and installing dependencies before copying the source code. This improves build caching when only source code changes.
The current approach is acceptable, but you could optimize further:
COPY pyproject.toml ./ RUN uv sync --no-dev COPY ./src ./srcThis is already properly structured for layer caching.
15-15: Consider using a more generic approach for model file deployment.Hard-coding the model file path in the Dockerfile makes it less flexible for different model versions. Consider using environment variables or a mounted volume for model files.
-COPY ./miniature_temporal_fusion_transformer.safetensor ./src/predictionengine/miniature_temporal_fusion_transformer.safetensor +COPY ./models/ ./src/predictionengine/models/Alternatively, use an environment variable to specify the model file path at runtime.
1-20: Consider adding security improvements.The Dockerfile could benefit from security enhancements such as running as a non-root user and adding a health check.
FROM python:3.12 COPY --from=ghcr.io/astral-sh/uv:latest /uv /bin/uv +# Create non-root user +RUN groupadd -r appuser && useradd -r -g appuser appuser + ENV PYTHONPATH=/app/src WORKDIR /app COPY pyproject.toml ./ RUN uv sync --no-dev COPY ./src ./src COPY ./miniature_temporal_fusion_transformer.safetensor ./src/predictionengine/miniature_temporal_fusion_transformer.safetensor +# Change ownership to non-root user +RUN chown -R appuser:appuser /app +USER appuser + EXPOSE 8080 +# Add health check +HEALTHCHECK --interval=30s --timeout=30s --start-period=5s --retries=3 \ + CMD curl -f http://localhost:8080/health || exit 1 + ENTRYPOINT ["uv", "run", "uvicorn", "predictionengine.main:application", "--host", "0.0.0.0", "--port", "8080", "--app-dir", "src"]application/predictionengine/tests/test_long_short_term_memory.py (1)
18-29: Verify hidden state structure assumptions.The test assumes the hidden state is a tuple with 2 elements, which is correct for LSTM (hidden state and cell state). However, consider adding more specific assertions about the hidden state shapes.
def test_lstm_forward(): lstm = LongShortTermMemory( input_size=16, hidden_size=32, layer_count=1, dropout_rate=0.0 ) input_tensor = Tensor(np.random.randn(4, 10, 16)) output, hidden_state = lstm.forward(input_tensor) assert output.shape == (4, 10, 32) assert isinstance(hidden_state, tuple) assert len(hidden_state) == 2 + # Verify hidden state shapes: (h_n, c_n) + h_n, c_n = hidden_state + assert h_n.shape == (4, 32) # (batch_size, hidden_size) + assert c_n.shape == (4, 32) # (batch_size, hidden_size)application/predictionengine/tests/test_gated_residual_network.py (1)
1-63: Consider adding edge case tests.The current test suite is comprehensive, but could benefit from additional edge cases such as testing with very small or large tensor values, or verifying the gating mechanism behavior.
Consider adding tests for:
def test_gated_residual_network_extreme_values(): grn = GatedResidualNetwork(input_size=4, hidden_size=8, output_size=4) # Test with very small values small_input = Tensor(np.full((2, 4), 1e-6)) small_output = grn.forward(small_input) assert small_output.shape == (2, 4) assert not np.any(np.isnan(small_output.numpy())) # Test with larger values large_input = Tensor(np.full((2, 4), 100.0)) large_output = grn.forward(large_input) assert large_output.shape == (2, 4) assert not np.any(np.isnan(large_output.numpy()))application/predictionengine/src/predictionengine/post_processor.py (2)
23-27: Simplify verbose numpy array type hints.The return type annotation is overly verbose and could be simplified for better readability.
- ) -> Tuple[ - np.ndarray[Any, np.dtype[np.float64]], - np.ndarray[Any, np.dtype[np.float64]], - np.ndarray[Any, np.dtype[np.float64]], - ]: + ) -> Tuple[np.ndarray, np.ndarray, np.ndarray]:
28-34: Consider optimizing the ticker decoding process.The current implementation converts to Polars DataFrame, then to Pandas, which may be inefficient for large datasets.
- decoded_tickers = self.ticker_encoder.inverse_transform( - pl.DataFrame( - { - "ticker": encoded_tickers, - } - ).to_pandas() - )["ticker"] + decoded_tickers = self.ticker_encoder.inverse_transform( + {"ticker": encoded_tickers} + )["ticker"]application/predictionengine/src/predictionengine/long_short_term_memory.py (2)
19-21: Avoid variable shadowing for clarity.The parameter
input_sizeis shadowed by the local variable assignment, which can be confusing.Use a different variable name:
self.layers: List[LSTMCell] = [] for index in range(layer_count): - input_size = input_size if index == 0 else self.hidden_size - self.layers.append(LSTMCell(input_size, self.hidden_size)) + layer_input_size = input_size if index == 0 else self.hidden_size + self.layers.append(LSTMCell(layer_input_size, self.hidden_size))
36-36: Remove unnecessary type conversion.The
sequence_lengthfrominput.shapeis already an integer.- for t in range(int(sequence_length)): + for t in range(sequence_length):application/predictionengine/src/predictionengine/dataset.py (3)
117-118: Remove confusing type annotation redefinition.Redefining
self.scalerswith a type annotation inside the method is unnecessary and confusing.if self.scalers is None or len(self.scalers) == 0: - self.scalers: Dict[str, Dict[str, Tensor]] = {} + self.scalers = {}
177-177: Remove redundant Tensor conversion.
batch_datais already a Tensor, so wrapping it inTensor()is unnecessary.- batch_data = Tensor(batch_data).stack(padding, dim=0) + batch_data = batch_data.stack(padding, dim=0)
164-193: Consider adding documentation for the batches generator.The batches method has complex logic for extracting tickers, historical features, and targets. Adding a docstring would improve maintainability.
Add a docstring explaining:
- The structure of yielded tensors (tickers, historical_features, targets)
- How padding is handled for incomplete batches
- The shape of each returned tensor
application/predictionengine/src/predictionengine/multi_head_self_attention.py (1)
53-53: Remove redundant Tensor creation.The
attention_scoresvariable is already a Tensor from the previous calculation, so wrapping it inTensor()is unnecessary.- attention_weights: Tensor = Tensor(attention_scores).softmax(axis=-1) + attention_weights: Tensor = attention_scores.softmax(axis=-1)application/predictionengine/src/predictionengine/main.py (1)
69-81: Consider making model hyperparameters configurable.The model hyperparameters are currently hardcoded. Consider making them configurable through environment variables or a configuration file for better flexibility across different deployment scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/test.yaml(1 hunks).mise.toml(1 hunks)Dockerfile.test(1 hunks)application/datamanager/Dockerfile(1 hunks)application/datamanager/pyproject.toml(1 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/Dockerfile(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)application/positionmanager/src/positionmanager/main.py(2 hunks)application/predictionengine/Dockerfile(1 hunks)application/predictionengine/pyproject.toml(1 hunks)application/predictionengine/src/predictionengine/dataset.py(1 hunks)application/predictionengine/src/predictionengine/gated_residual_network.py(1 hunks)application/predictionengine/src/predictionengine/long_short_term_memory.py(1 hunks)application/predictionengine/src/predictionengine/loss_function.py(1 hunks)application/predictionengine/src/predictionengine/main.py(1 hunks)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py(1 hunks)application/predictionengine/src/predictionengine/models.py(1 hunks)application/predictionengine/src/predictionengine/multi_head_self_attention.py(1 hunks)application/predictionengine/src/predictionengine/post_processor.py(1 hunks)application/predictionengine/src/predictionengine/ticker_embedding.py(1 hunks)application/predictionengine/tests/test_dataset.py(1 hunks)application/predictionengine/tests/test_gated_residual_network.py(1 hunks)application/predictionengine/tests/test_long_short_term_memory.py(1 hunks)application/predictionengine/tests/test_loss_function.py(1 hunks)application/predictionengine/tests/test_multi_head_self_attention.py(1 hunks)application/predictionengine/tests/test_post_processor.py(1 hunks)application/predictionengine/tests/test_ticker_embedding.py(1 hunks)compose.yaml(1 hunks)pyproject.toml(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
application/positionmanager/src/positionmanager/main.py (1)
application/positionmanager/src/positionmanager/clients.py (1)
DataClient(61-101)
application/predictionengine/tests/test_loss_function.py (1)
application/predictionengine/src/predictionengine/loss_function.py (1)
quantile_loss(4-10)
application/predictionengine/tests/test_dataset.py (1)
application/predictionengine/src/predictionengine/dataset.py (5)
DataSet(18-193)load_data(42-144)get_preprocessors(146-162)batches(164-193)validate_preprocessors(34-37)
application/positionmanager/src/positionmanager/clients.py (1)
application/positionmanager/src/positionmanager/models.py (2)
Money(7-33)from_float(29-30)
application/predictionengine/tests/test_post_processor.py (1)
application/predictionengine/src/predictionengine/post_processor.py (2)
PostProcessor(8-47)post_process_predictions(19-47)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (7)
application/predictionengine/src/predictionengine/ticker_embedding.py (2)
TickerEmbedding(5-10)forward(9-10)application/predictionengine/src/predictionengine/long_short_term_memory.py (2)
LongShortTermMemory(6-57)forward(23-57)application/predictionengine/src/predictionengine/gated_residual_network.py (2)
GatedResidualNetwork(5-29)forward(19-29)application/predictionengine/src/predictionengine/multi_head_self_attention.py (2)
MultiHeadSelfAttention(7-63)forward(29-63)application/predictionengine/src/predictionengine/post_processor.py (2)
PostProcessor(8-47)post_process_predictions(19-47)application/predictionengine/src/predictionengine/dataset.py (1)
batches(164-193)application/predictionengine/src/predictionengine/loss_function.py (1)
quantile_loss(4-10)
🪛 Hadolint (2.12.0)
Dockerfile.test
[error] 10-10: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
🪛 Ruff (0.11.9)
application/predictionengine/src/predictionengine/loss_function.py
4-4: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
application/predictionengine/src/predictionengine/main.py
160-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
application/predictionengine/src/predictionengine/dataset.py
24-24: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (37)
application/positionmanager/Dockerfile (2)
1-1: LGTM! Python version standardization.The downgrade from Python 3.13 to 3.12 aligns with the project-wide standardization effort mentioned in the AI summary.
10-10: LGTM! Formatting improvement.The added blank line improves readability and consistency with other Dockerfiles in the project.
application/datamanager/Dockerfile (2)
1-2: LGTM! Consistent Python version standardization.The change to Python 3.12 and added formatting aligns with the broader project updates for consistency across all services.
8-8: LGTM! Formatting consistency.The blank line addition improves readability and maintains consistency with other Dockerfiles.
application/datamanager/pyproject.toml (1)
14-15: LGTM! Good observability dependencies added.The addition of
prometheus-fastapi-instrumentatorfor metrics andlogurufor enhanced logging will improve the service's observability and debugging capabilities. Both are well-established libraries with appropriate version constraints.application/predictionengine/src/predictionengine/ticker_embedding.py (1)
5-10: LGTM! Clean embedding implementation.The
TickerEmbeddingclass is well-implemented with proper type hints and follows standard neural network component patterns. The use of tinygrad is appropriate for this lightweight embedding layer.Dockerfile.test (3)
14-14: LGTM! Coverage directory setup.Creating the coverage output directory aligns well with the updated coverage workflow in the GitHub Actions.
10-10: Static analysis hint is incorrect.The COPY command syntax
COPY pyproject.toml uv.lock .is valid for copying multiple files to the current directory. The static analysis tool's suggestion about requiring a trailing slash is incorrect for this use case.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 10-10: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
12-12:Details
❓ Verification inconclusive
Verify the uv sync flag change.
The flag changed from
--all-groupsto--all-packages. Please verify this is the correct flag for your uv version and intended behavior.
🏁 Script executed:
#!/bin/bash # Verify uv sync command options and check if --all-packages is valid uv sync --help | grep -E "(all-groups|all-packages)"Length of output: 147
Verify uv sync flag in Dockerfile.test
At Dockerfile.test:12 the command was changed fromuv sync --all-groupstouv sync --all-packages. Sinceuvisn’t available locally to confirm flag names, please:
- Build or run the test image and execute
uv sync --helpinside the container to ensure--all-packagesis valid.- Alternatively, consult your uv CLI documentation or release notes to confirm that
--all-packagesreplaces--all-groupswith the same behavior..github/workflows/test.yaml (1)
22-22: LGTM! Coverage path update aligns with new structure.The path change to
coverage_output/.python_coverage.xmlcorrectly aligns with the coverage directory structure established in Dockerfile.test and other configuration files..mise.toml (2)
32-32: Good practice: Adding dependency ensures reliable test execution.Adding the dependency on
python:installensures that Python dependencies are properly installed before running tests, which improves build reliability.
35-35: Supports coverage output restructuring.Creating the
coverage_outputdirectory aligns with the coordinated changes across Docker, CI, and test configurations to isolate coverage output.compose.yaml (2)
9-11: Improved container isolation with specific volume mounts.The change from mounting the entire current directory to specific mounts for
applicationandcoverage_outputis a good improvement that avoids conflicts with local virtual environments and provides better isolation.
19-19: Explicit coverage output path ensures consistency.Specifying the exact coverage output file path ensures the coverage report is written to the expected location that aligns with the new directory structure.
application/predictionengine/pyproject.toml (2)
5-5: Clarify Python version requirement.The comment suggests "possibly 3.10" but the requirement specifies
>=3.12. Please clarify the actual minimum Python version needed and update either the comment or the requirement for consistency.
1-23: Well-structured project configuration.The overall project structure, dependency selection, and build system configuration are appropriate for a FastAPI-based prediction engine service.
application/positionmanager/src/positionmanager/clients.py (2)
26-31: Excellent defensive programming for cash balance handling.The explicit validation of the
cashattribute prevents potential runtime errors when the account object doesn't contain expected cash balance data. This improvement enhances robustness and provides clear error messaging.
97-97: Good API modernization for Polars DataFrame pivot.The update from
columns="ticker"toon="ticker"aligns with newer Polars API conventions while maintaining the same pivot functionality by date and ticker with close prices as values.application/predictionengine/tests/test_ticker_embedding.py (1)
7-56: Comprehensive and well-structured test suite.The test coverage is excellent, validating:
- Proper initialization of the embedding layer
- Forward pass functionality with single and batch inputs
- Shape correctness across different embedding sizes
- Range validation for all valid ticker IDs
- Consistency between batch and individual processing
This thorough testing approach ensures the
TickerEmbeddingcomponent behaves correctly across various scenarios.pyproject.toml (4)
12-12: Proper integration of prediction engine workspace member.The addition of
application/predictionenginecorrectly integrates the new prediction engine component into the workspace structure.
19-19: Consistent workspace source configuration.Adding
predictionengine = { workspace = true }maintains consistency with other workspace members and enables proper dependency resolution.
40-40: Updated test paths align with new component structure.Including
application/predictionengine/testsin testpaths ensures the new prediction engine tests are properly discovered and executed by pytest.
52-52: Improved coverage output organization.Moving coverage output to
coverage_output/.python_coverage.xmlprovides better organization and separation of build artifacts from source code.application/predictionengine/tests/test_long_short_term_memory.py (2)
8-16: LGTM! Comprehensive initialization testing.The initialization test properly verifies that the LSTM stores the expected parameters correctly.
66-78: Good consistency test with appropriate tolerance.The consistency test properly verifies deterministic behavior when dropout is disabled. The tolerance
rtol=1e-5is appropriate for floating-point comparisons.application/predictionengine/tests/test_gated_residual_network.py (3)
8-24: Excellent initialization testing with detailed shape verification.The test properly verifies the internal dense layer and gate weight shapes, ensuring the network is constructed correctly with the expected parameter dimensions.
26-33: Good forward pass testing.The test correctly verifies that the output shape matches the expected dimensions for the given input.
53-63: Strong consistency testing.The consistency test properly verifies deterministic behavior across repeated forward passes with the same input, which is crucial for reproducible results.
application/predictionengine/tests/test_multi_head_self_attention.py (1)
8-57: Excellent test coverage with comprehensive scenarios.The test suite effectively covers:
- Initialization parameter validation
- Forward pass output shape verification
- Multiple head configurations
- Edge cases (single sequence, longer sequences)
- Batch processing with different sizes
The tests are well-structured and provide good confidence in the MultiHeadSelfAttention implementation.
application/predictionengine/tests/test_dataset.py (4)
6-17: LGTM: Initialization test correctly verifies dataset setup.The test properly validates the dataset initialization parameters and the calculated length based on batch size and sample count.
19-52: LGTM: Comprehensive data loading test with proper assertions.The test creates realistic financial data and verifies that the dataset correctly:
- Loads and processes the data
- Creates necessary preprocessors (indices, ticker_encoder)
- Populates internal data structures
83-113: LGTM: Thorough batch generation testing.The test properly validates that:
- Batches are generated correctly
- Tensor shapes match expected dimensions
- Multiple batches are produced
115-123: LGTM: Proper validation testing for edge cases.The test correctly verifies that the
validate_preprocessorsmethod raises appropriate errors for invalid input, ensuring robust error handling.application/predictionengine/src/predictionengine/post_processor.py (1)
43-47: LGTM: Percentile computation is correct and efficient.The percentile calculations properly compute the 25th, 50th, and 75th percentiles along the last axis, providing appropriate uncertainty quantification for the predictions.
application/predictionengine/tests/test_post_processor.py (2)
10-24: LGTM! Test correctly verifies PostProcessor initialization.The test properly validates that the PostProcessor stores the provided means, standard deviations, and ticker encoder.
26-75: Well-structured test for multi-ticker prediction post-processing.The test effectively validates:
- Proper ticker encoding/decoding workflow
- Correct rescaling of predictions using ticker-specific means and standard deviations
- Percentile ordering constraints (25th ≤ 50th ≤ 75th)
application/predictionengine/src/predictionengine/long_short_term_memory.py (1)
51-54: Review the training mode setting within forward pass.Setting
.train()mode within the forward pass is unconventional. The model's training/evaluation mode should typically be managed at a higher level (e.g., before calling forward).Consider removing the
.train()call and ensuring the model's mode is set appropriately before the forward pass. The dropout behavior should automatically adjust based on the model's global training state.if self.dropout_rate > 0.0 and index < self.layer_count - 1: - hidden_state[index].train() hidden_state[index] = hidden_state[index].dropout(self.dropout_rate)
|
|
||
| class Polygon(BaseModel): | ||
| api_key: str = Field(default=os.getenv("POLYGON_API_KEY")) | ||
| api_key: str = Field(default=os.getenv("POLYGON_API_KEY", "")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validation for required configuration fields.
While defaulting to empty strings provides type consistency, it may mask configuration errors. Consider adding validation to ensure critical fields like API keys and project identifiers are properly configured.
Also note the inconsistency: credentials_path on line 24 still uses the old pattern without an empty string default.
class GCP(BaseModel):
bucket: Bucket = Bucket()
- credentials_path: str = os.getenv("GOOGLE_APPLICATION_CREDENTIALS", "")
+ credentials_path: str = Field(default=os.getenv("GOOGLE_APPLICATION_CREDENTIALS", ""))Consider adding validation to ensure required fields are non-empty:
from pydantic import BaseModel, Field, field_validator
class Polygon(BaseModel):
api_key: str = Field(default=os.getenv("POLYGON_API_KEY", ""))
@field_validator('api_key')
@classmethod
def validate_api_key(cls, v):
if not v:
raise ValueError('POLYGON_API_KEY environment variable is required')
return vAlso applies to: 14-15
🤖 Prompt for AI Agents
In application/datamanager/src/datamanager/config.py at line 8 and also lines
14-15 and 24, the current use of default empty strings for critical config
fields like api_key and credentials_path can hide missing configuration errors.
To fix this, add pydantic field validators for these fields that check if the
value is non-empty and raise a ValueError if not. This ensures required
environment variables are properly set and prevents silent misconfiguration.
| api_key=os.getenv("ALPACA_API_KEY", ""), | ||
| api_secret=os.getenv("ALPACA_API_SECRET", ""), |
There was a problem hiding this comment.
Avoid masking configuration errors with empty string defaults.
Providing empty string defaults for critical configuration like API keys and service URLs masks configuration errors instead of failing fast. The DataClient.get_data method explicitly checks for empty datamanager_base_url and raises a ValueError, and empty API keys will cause authentication failures at runtime.
Consider these better approaches:
Option 1: Validate at startup
+def validate_config():
+ required_vars = ["ALPACA_API_KEY", "ALPACA_API_SECRET", "DATAMANAGER_BASE_URL"]
+ missing = [var for var in required_vars if not os.getenv(var)]
+ if missing:
+ raise ValueError(f"Missing required environment variables: {missing}")
+
+@application.on_event("startup")
+async def startup_event():
+ validate_config()Option 2: Keep None and handle explicitly
- api_key=os.getenv("ALPACA_API_KEY", ""),
- api_secret=os.getenv("ALPACA_API_SECRET", ""),
+ api_key=os.getenv("ALPACA_API_KEY") or raise ValueError("ALPACA_API_KEY not set"),
+ api_secret=os.getenv("ALPACA_API_SECRET") or raise ValueError("ALPACA_API_SECRET not set"),Also applies to: 31-31
🤖 Prompt for AI Agents
In application/positionmanager/src/positionmanager/main.py around lines 26 to 27
and line 31, remove the empty string defaults from the os.getenv calls for
critical configuration values like API keys and service URLs. Instead, allow
these to be None if not set, and add explicit validation at startup to check for
missing environment variables, raising an error immediately if any are absent.
This prevents masking configuration errors and ensures the application fails
fast with clear error messages.
| def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]): | ||
| loss = Tensor.zeros(1) | ||
| for q in quantiles: | ||
| error = y_true - y_pred | ||
| loss += Tensor.maximum(q * error, (q - 1) * error).mean() | ||
|
|
||
| return loss |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation.
The function lacks input validation and documentation, which could lead to runtime errors or confusion about its purpose and usage.
Consider adding input validation and comprehensive documentation:
+def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None):
+ """
+ Compute quantile loss for predicted and true tensor values.
+
+ Args:
+ y_pred: Predicted values tensor
+ y_true: True values tensor
+ quantiles: List of quantiles to compute loss for (default: [0.25, 0.5, 0.75])
+
+ Returns:
+ Tensor: Accumulated quantile loss
+
+ Raises:
+ ValueError: If tensors have incompatible shapes or quantiles are invalid
+ """
+ if quantiles is None:
+ quantiles = [0.25, 0.5, 0.75]
+
+ if y_pred.shape != y_true.shape:
+ raise ValueError(f"Shape mismatch: y_pred {y_pred.shape} vs y_true {y_true.shape}")
+
+ if not all(0 <= q <= 1 for q in quantiles):
+ raise ValueError("All quantiles must be between 0 and 1")
+
-def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]):
loss = Tensor.zeros(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]): | |
| loss = Tensor.zeros(1) | |
| for q in quantiles: | |
| error = y_true - y_pred | |
| loss += Tensor.maximum(q * error, (q - 1) * error).mean() | |
| return loss | |
| def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None): | |
| """ | |
| Compute quantile loss for predicted and true tensor values. | |
| Args: | |
| y_pred: Predicted values tensor | |
| y_true: True values tensor | |
| quantiles: List of quantiles to compute loss for (default: [0.25, 0.5, 0.75]) | |
| Returns: | |
| Tensor: Accumulated quantile loss | |
| Raises: | |
| ValueError: If tensors have incompatible shapes or quantiles are invalid | |
| """ | |
| if quantiles is None: | |
| quantiles = [0.25, 0.5, 0.75] | |
| if y_pred.shape != y_true.shape: | |
| raise ValueError(f"Shape mismatch: y_pred {y_pred.shape} vs y_true {y_true.shape}") | |
| if not all(0 <= q <= 1 for q in quantiles): | |
| raise ValueError("All quantiles must be between 0 and 1") | |
| loss = Tensor.zeros(1) | |
| for q in quantiles: | |
| error = y_true - y_pred | |
| loss += Tensor.maximum(q * error, (q - 1) * error).mean() | |
| return loss |
🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/loss_function.py around
lines 4 to 10, the quantile_loss function lacks input validation and
documentation. Add a docstring explaining the function's purpose, parameters,
and return value. Implement input validation to check that y_pred and y_true are
tensors of compatible shapes and that quantiles is a list of floats within the
range [0,1]. Raise appropriate errors if validations fail to prevent runtime
issues.
| for tickers_batch, features_batch, _ in dataset.batches(): | ||
| percentile_25, percentile_50, percentile_75 = model.predict( | ||
| tickers_batch, features_batch | ||
| ) | ||
|
|
||
| predictions[ticker] = { | ||
| "percentile_25": float(percentile_25[0]), | ||
| "percentile_50": float(percentile_50[0]), | ||
| "percentile_75": float(percentile_75[0]), | ||
| } | ||
| break | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the batch iteration logic.
Since the dataset is configured with batch_size=1 and sample_count=1, there will only be one batch. The nested loop structure is unnecessary and confusing.
- for tickers_batch, features_batch, _ in dataset.batches():
- percentile_25, percentile_50, percentile_75 = model.predict(
- tickers_batch, features_batch
- )
-
- predictions[ticker] = {
- "percentile_25": float(percentile_25[0]),
- "percentile_50": float(percentile_50[0]),
- "percentile_75": float(percentile_75[0]),
- }
- break
+ # Get the single batch
+ batch_generator = dataset.batches()
+ tickers_batch, features_batch, _ = next(batch_generator)
+
+ percentile_25, percentile_50, percentile_75 = model.predict(
+ tickers_batch, features_batch
+ )
+
+ predictions[ticker] = {
+ "percentile_25": float(percentile_25[0]),
+ "percentile_50": float(percentile_50[0]),
+ "percentile_75": float(percentile_75[0]),
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for tickers_batch, features_batch, _ in dataset.batches(): | |
| percentile_25, percentile_50, percentile_75 = model.predict( | |
| tickers_batch, features_batch | |
| ) | |
| predictions[ticker] = { | |
| "percentile_25": float(percentile_25[0]), | |
| "percentile_50": float(percentile_50[0]), | |
| "percentile_75": float(percentile_75[0]), | |
| } | |
| break | |
| # Get the single batch | |
| batch_generator = dataset.batches() | |
| tickers_batch, features_batch, _ = next(batch_generator) | |
| percentile_25, percentile_50, percentile_75 = model.predict( | |
| tickers_batch, features_batch | |
| ) | |
| predictions[ticker] = { | |
| "percentile_25": float(percentile_25[0]), | |
| "percentile_50": float(percentile_50[0]), | |
| "percentile_75": float(percentile_75[0]), | |
| } |
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/main.py around lines 136 to
147, the code uses a for loop to iterate over dataset batches, but since
batch_size and sample_count are both 1, there is only one batch. Replace the for
loop with a direct call to get the single batch from the dataset, then perform
the prediction and assignment without looping, simplifying the logic and
improving readability.
| batch_count = len(dataset) | ||
|
|
||
| for batch in dataset.batches(): | ||
| tickers, features, targets = batch | ||
| tickers, features, targets = ( | ||
| Tensor(tickers), | ||
| Tensor(features), | ||
| Tensor(targets), | ||
| ) |
There was a problem hiding this comment.
Fix the batch iteration in validation method.
The batch iteration has the same issue as in the train method. Also, len(dataset) might not return the correct number of batches.
def validate(
self,
dataset: DataSet,
) -> float:
total_loss = 0.0
- batch_count = len(dataset)
+ batch_count = 0
- for batch in dataset.batches():
- tickers, features, targets = batch
+ for tickers, features, targets in dataset.batches():
tickers, features, targets = (
Tensor(tickers),
Tensor(features),
Tensor(targets),
)
+ batch_count += 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| batch_count = len(dataset) | |
| for batch in dataset.batches(): | |
| tickers, features, targets = batch | |
| tickers, features, targets = ( | |
| Tensor(tickers), | |
| Tensor(features), | |
| Tensor(targets), | |
| ) | |
| def validate( | |
| self, | |
| dataset: DataSet, | |
| ) -> float: | |
| total_loss = 0.0 | |
| - batch_count = len(dataset) | |
| + batch_count = 0 | |
| - for batch in dataset.batches(): | |
| - tickers, features, targets = batch | |
| + for tickers, features, targets in dataset.batches(): | |
| tickers, features, targets = ( | |
| Tensor(tickers), | |
| Tensor(features), | |
| Tensor(targets), | |
| ) | |
| + batch_count += 1 | |
| # ... rest of validation logic using total_loss and batch_count ... |
🤖 Prompt for AI Agents
In
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
around lines 142 to 150, the batch iteration in the validation method
incorrectly uses len(dataset) for batch count and iterates over
dataset.batches() similarly to the train method, which is problematic. Fix this
by using the correct method or property to get the number of batches and ensure
the batch iteration matches the correct approach used in the train method,
properly unpacking and converting batch elements to Tensors.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
application/predictionengine/src/predictionengine/dataset.py (2)
8-15: Consider makingcontinuous_variable_columnsa class constant.To improve encapsulation and make the code more maintainable, consider moving this global variable into the
DataSetclass as a class constant.-continuous_variable_columns = [ - "open_price", - "high_price", - "low_price", - "close_price", - "volume", - "volume_weighted_average_price", -] - - class DataSet(BaseModel): + CONTINUOUS_VARIABLE_COLUMNS = [ + "open_price", + "high_price", + "low_price", + "close_price", + "volume", + "volume_weighted_average_price", + ] +Then update all references from
continuous_variable_columnstoself.CONTINUOUS_VARIABLE_COLUMNS.
146-155: Remove unnecessary parentheses around dictionary comprehensions.The parentheses around the dictionary comprehensions are unnecessary and could be confusing.
def get_preprocessors(self) -> Dict[str, Any]: - means_by_ticker = ( - {ticker: values["means"] for ticker, values in self.scalers.items()} - ) - standard_deviations_by_ticker = ( - { - ticker: values["standard_deviations"] - for ticker, values in self.scalers.items() - } - ) + means_by_ticker = { + ticker: values["means"] for ticker, values in self.scalers.items() + } + standard_deviations_by_ticker = { + ticker: values["standard_deviations"] + for ticker, values in self.scalers.items() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
application/predictionengine/src/predictionengine/dataset.py(1 hunks)application/predictionengine/tests/test_post_processor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- application/predictionengine/tests/test_post_processor.py
| def load_data(self, data: pl.DataFrame) -> None: | ||
| data = data.with_columns( | ||
| [ | ||
| pl.col("timestamp").str.strptime(pl.Datetime).cast(pl.Date), | ||
| pl.col("open_price").cast(pl.Float64), | ||
| pl.col("high_price").cast(pl.Float64), | ||
| pl.col("low_price").cast(pl.Float64), | ||
| pl.col("close_price").cast(pl.Float64), | ||
| pl.col("volume").cast(pl.Float64), | ||
| pl.col("volume_weighted_average_price").cast(pl.Float64), | ||
| pl.col("ticker").cast(pl.Utf8), | ||
| ] | ||
| ) | ||
|
|
||
| self.preprocessors["indices"] = { | ||
| col: idx for idx, col in enumerate(data.columns) | ||
| } | ||
|
|
||
| data = data.unique(subset=["ticker", "timestamp"]) | ||
|
|
||
| tickers = data.select("ticker").unique() | ||
| minimum_timestamp = data.select(pl.col("timestamp").min())[0, 0] | ||
| maximum_timestamp = data.select(pl.col("timestamp").max())[0, 0] | ||
|
|
||
| full_dates = pl.DataFrame( | ||
| { | ||
| "timestamp": pl.date_range( | ||
| start=minimum_timestamp, | ||
| end=maximum_timestamp, | ||
| interval="1d", | ||
| closed="both", | ||
| eager=True, | ||
| ) | ||
| } | ||
| ) | ||
|
|
||
| full_tickers_and_dates = tickers.join(full_dates, how="cross") | ||
|
|
||
| full_tickers_and_dates = full_tickers_and_dates.with_columns( | ||
| [ | ||
| pl.col("timestamp") | ||
| .rank(method="dense") | ||
| .cast(pl.Int32) | ||
| .alias("time_index") | ||
| - 1 | ||
| ] | ||
| ) | ||
|
|
||
| data = full_tickers_and_dates.join(data, on=["ticker", "timestamp"], how="left") | ||
|
|
||
| data = data.sort(["ticker", "timestamp"]) | ||
|
|
||
| data = data.group_by("ticker").map_groups( | ||
| lambda df: df.sort("timestamp").with_columns( | ||
| [ | ||
| pl.col(col) | ||
| .interpolate() | ||
| .fill_null(strategy="forward") | ||
| .fill_null(strategy="backward") | ||
| for col in (continuous_variable_columns) | ||
| ] | ||
| ) | ||
| ) | ||
|
|
||
| ticker_series = data["ticker"].to_numpy() | ||
| ticker_encoder = OrdinalEncoder( | ||
| cols=["ticker"], | ||
| handle_unknown="use_encoded_value", | ||
| handle_missing="use_encoded_value", | ||
| ) | ||
| self.preprocessors["ticker_encoder"] = ticker_encoder | ||
| encoded_tickers = ticker_encoder.fit_transform(ticker_series) | ||
|
|
||
| data = data.with_columns(pl.Series("ticker", encoded_tickers["ticker"])) | ||
|
|
||
| if self.scalers is None or len(self.scalers) == 0: | ||
| self.scalers: Dict[str, Dict[str, Tensor]] = {} | ||
| for ticker_key, group in data.group_by("ticker"): | ||
| ticker = ticker_key[0] | ||
| means = group[continuous_variable_columns].mean() | ||
| standard_deviations = group[continuous_variable_columns].std() | ||
|
|
||
| self.scalers[str(ticker)] = { | ||
| "means": Tensor(means.to_numpy()), | ||
| "standard_deviations": Tensor(standard_deviations.to_numpy()), | ||
| } | ||
|
|
||
| groups: List[Tensor] = [] | ||
| for ticker_key, group in data.group_by("ticker"): | ||
| ticker = ticker_key[0] | ||
| means = self.scalers[str(ticker)]["means"] | ||
| standard_deviations = self.scalers[str(ticker)]["standard_deviations"] | ||
|
|
||
| group_data = Tensor(group.select(continuous_variable_columns).to_numpy()) | ||
|
|
||
| scaled_group = (group_data.sub(means)).div(standard_deviations) | ||
|
|
||
| groups.append(scaled_group) | ||
|
|
||
| output_data = Tensor.empty(groups[0].shape) | ||
| output_data = output_data.cat(*groups, dim=0) | ||
|
|
||
| self.data = output_data | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the load_data method for better maintainability.
This method is doing too many things: type casting, date range generation, interpolation, encoding, scaling, and concatenation. Consider breaking it down into smaller, focused methods.
Example refactoring approach:
_cast_columns()- Handle type casting_generate_complete_time_series()- Create full date ranges_encode_tickers()- Handle ticker encoding_compute_scalers()- Calculate means and standard deviations_scale_data()- Apply scaling transformations
Would you like me to generate the refactored implementation?
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/dataset.py from lines 42 to
145, the load_data method is handling multiple responsibilities such as type
casting, date range generation, interpolation, encoding, scaling, and
concatenation, which reduces maintainability. Refactor this method by extracting
these distinct tasks into separate private methods named _cast_columns for type
casting, _generate_complete_time_series for creating full date ranges,
_encode_tickers for ticker encoding, _compute_scalers for calculating means and
standard deviations, and _scale_data for applying scaling transformations. Then,
update load_data to call these smaller focused methods in sequence to improve
readability and maintainability.
| output_data = Tensor.empty(groups[0].shape) | ||
| output_data = output_data.cat(*groups, dim=0) |
There was a problem hiding this comment.
Add error handling for empty groups list.
If no data remains after grouping (e.g., all tickers are filtered out), groups[0] will raise an IndexError.
+ if not groups:
+ raise ValueError("No data available after preprocessing")
+
output_data = Tensor.empty(groups[0].shape)
output_data = output_data.cat(*groups, dim=0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output_data = Tensor.empty(groups[0].shape) | |
| output_data = output_data.cat(*groups, dim=0) | |
| if not groups: | |
| raise ValueError("No data available after preprocessing") | |
| output_data = Tensor.empty(groups[0].shape) | |
| output_data = output_data.cat(*groups, dim=0) |
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/dataset.py around lines 141
to 142, the code assumes the groups list is non-empty, but if it is empty,
accessing groups[0] causes an IndexError. Add a check before this line to verify
if groups is empty; if so, handle this case appropriately by either returning an
empty tensor or raising a clear exception. This prevents the code from crashing
unexpectedly when no data remains after filtering.
| if self.scalers is None or len(self.scalers) == 0: | ||
| self.scalers: Dict[str, Dict[str, Tensor]] = {} |
There was a problem hiding this comment.
Fix incorrect scalers check and remove redundant type annotation.
The condition self.scalers is None will never be true since self.scalers is initialized as {} in __init__. Also, re-declaring the instance variable with a type annotation is unnecessary and confusing.
- if self.scalers is None or len(self.scalers) == 0:
- self.scalers: Dict[str, Dict[str, Tensor]] = {}
+ if len(self.scalers) == 0:Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/dataset.py around lines 117
to 118, remove the check for `self.scalers is None` since `self.scalers` is
always initialized as an empty dictionary in `__init__`. Also, eliminate the
redundant type annotation on `self.scalers` when assigning it to an empty
dictionary to avoid confusion and maintain clarity.
| *batch_data.shape[1:], | ||
| ) | ||
| ) | ||
| batch_data = Tensor(batch_data).stack(padding, dim=0) |
There was a problem hiding this comment.
Fix incorrect tensor operation - use cat instead of stack.
Tensor.stack creates a new dimension, but here you want to concatenate along the existing dimension.
- batch_data = Tensor(batch_data).stack(padding, dim=0)
+ batch_data = batch_data.cat(padding, dim=0)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/dataset.py at line 177,
replace the incorrect use of Tensor.stack with Tensor.cat to concatenate
batch_data along the existing dimension instead of creating a new one. Change
the method call from stack to cat and ensure the padding argument is correctly
used for concatenation along dim=0.
| tickers = batch_data[: self.batch_size, 0] | ||
|
|
||
| historical_features = Tensor.stack( | ||
| *[ | ||
| batch_data[i : i + self.sequence_length, 1:] | ||
| for i in range(self.batch_size) | ||
| ], | ||
| dim=0, | ||
| ) | ||
|
|
||
| targets = batch_data[: self.batch_size, close_price_idx].reshape( | ||
| self.batch_size, 1 | ||
| ) | ||
|
|
||
| yield tickers, historical_features, targets |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix incorrect assumptions about data structure in batches method.
The method assumes column 0 contains tickers, but after load_data, the tensor only contains continuous variables (prices and volume). The ticker information is lost during the scaling and concatenation process.
The current implementation has several issues:
- Tickers are not preserved in the final tensor
- Column indices don't match the expected structure
- The slicing logic assumes a structure that doesn't exist
Consider modifying load_data to preserve ticker information separately or restructure the data format to include all necessary information for batch generation.
🏁 Script executed:
#!/bin/bash
# Search for how the DataSet class is used in the codebase to understand expected behavior
ast-grep --pattern 'DataSet($$$)'
# Look for tests that might clarify the expected data structure
fd -e py test_dataset | xargs rg -A 10 "def test.*batch"Length of output: 3317
🏁 Script executed:
#!/bin/bash
# Display load_data implementation
rg -n "def load_data" -A 50 application/predictionengine/src/predictionengine/dataset.py
echo "----"
# Display batches generator
rg -n "def batches" -A 20 application/predictionengine/src/predictionengine/dataset.py
echo "----"
# Inspect test for batch output expectations
rg -n "def test_dataset_batches" -A 30 application/predictionengine/tests/test_dataset.pyLength of output: 4306
Fix column‐index mapping and slicing in batches
The code records self.preprocessors["indices"] before the DataFrame is reshaped—adding time_index and reordering columns—so numeric indices no longer line up with the final tensor. This causes:
close_price_idxto pull the wrong columnhistorical_featuresto include extra fields (timestamp, time_index) instead of just the 6 numeric features- Mismatch between expected feature count (6) and actual tensor shape (8)
Ticks are in fact still the first column after the join, so only the index mapping and slicing need adjustment.
Please update:
- In
application/predictionengine/src/predictionengine/dataset.py, after you build and sortself.data, recomputeself.preprocessors["indices"] = { col: idx for idx, col in enumerate(self.data.columns) }
- In the
batchesmethod, slice out exactly the numeric columns (open_price,high_price,low_price,close_price,volume,volume_weighted_average_price) by name or by the recomputed indices (so thathistorical_features.shape[-1] == 6).
Locations:
load_data: move or duplicate index‐map assignment right afterself.data = data.sort([...])batches: lines 164–183 (aroundclose_price_idxand the two tensor slices)
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/dataset.py around lines 164
to 183 and 179 to 193, the batch method incorrectly assumes column 0 contains
tickers and uses outdated column indices for slicing features and targets. To
fix this, after sorting and finalizing self.data in load_data, recompute
self.preprocessors["indices"] to map current column names to their correct
indices. Then, in batches, use these updated indices to slice only the numeric
feature columns (open_price, high_price, low_price, close_price, volume,
volume_weighted_average_price) for historical_features, ensuring its last
dimension is 6, and slice targets using the correct close_price index. Keep
tickers from the first column as they remain unchanged. This will align the data
structure assumptions with the actual tensor format.
forstmeier
force-pushed
the
05-25-build_initial_prediction_engine_service
branch
from
4172a2b to
07f5f0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
application/predictionengine/src/predictionengine/loss_function.py (2)
4-4:⚠️ Potential issueFix mutable default argument anti-pattern.
Using a mutable list as a default argument can lead to unexpected behavior when the function is called multiple times, as the same list object is reused across calls.
Apply this diff to fix the issue:
-def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]): +def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None): + if quantiles is None: + quantiles = [0.25, 0.5, 0.75]🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
4-10: 🛠️ Refactor suggestionAdd input validation and documentation.
The function lacks input validation and documentation, which could lead to runtime errors or confusion about its purpose and usage.
Consider adding input validation and comprehensive documentation:
+def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None): + """ + Compute quantile loss for predicted and true tensor values. + + Args: + y_pred: Predicted values tensor + y_true: True values tensor + quantiles: List of quantiles to compute loss for (default: [0.25, 0.5, 0.75]) + + Returns: + Tensor: Accumulated quantile loss + + Raises: + ValueError: If tensors have incompatible shapes or quantiles are invalid + """ + if quantiles is None: + quantiles = [0.25, 0.5, 0.75] + + if y_pred.shape != y_true.shape: + raise ValueError(f"Shape mismatch: y_pred {y_pred.shape} vs y_true {y_true.shape}") + + if not all(0 <= q <= 1 for q in quantiles): + raise ValueError("All quantiles must be between 0 and 1") + -def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]): loss = Tensor.zeros(1)🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
application/predictionengine/src/predictionengine/dataset.py (7)
42-145: Consider refactoring theload_datamethod for better maintainability.This method is doing too many things: type casting, date range generation, interpolation, encoding, scaling, and concatenation. Consider breaking it down into smaller, focused methods.
Example refactoring approach:
_cast_columns()- Handle type casting_generate_complete_time_series()- Create full date ranges_encode_tickers()- Handle ticker encoding_compute_scalers()- Calculate means and standard deviations_scale_data()- Apply scaling transformationsWould you like me to generate the refactored implementation?
32-37:⚠️ Potential issueRemove or fix the preprocessors validator - it prevents class instantiation.
The validator will always fail because
preprocessorsis initialized as an empty dict in__init__. This makes it impossible to create an instance of the class.Either remove the validator if preprocessors are meant to be populated later:
- @field_validator("preprocessors") - @classmethod - def validate_preprocessors(cls, v: Dict[str, Any]) -> Dict[str, Any]: - if v is None or len(v) == 0: - raise ValueError("Preprocessors cannot be None or empty") - return vOr modify it to skip validation for empty dicts during initialization:
@field_validator("preprocessors") @classmethod def validate_preprocessors(cls, v: Dict[str, Any]) -> Dict[str, Any]: - if v is None or len(v) == 0: - raise ValueError("Preprocessors cannot be None or empty") + # Allow empty dict during initialization, as it will be populated by load_data + if v is None: + raise ValueError("Preprocessors cannot be None") return v
117-118:⚠️ Potential issueFix incorrect scalers check.
The condition
self.scalers is Nonewill never be true sinceself.scalersis initialized as{}in__init__. Also, the type annotation is redundant.- if self.scalers is None or len(self.scalers) == 0: - self.scalers: Dict[str, Dict[str, Tensor]] = {} + if len(self.scalers) == 0:
141-142:⚠️ Potential issueAdd error handling for empty groups list.
If no data remains after grouping (e.g., all tickers are filtered out),
groups[0]will raise an IndexError.+ if not groups: + raise ValueError("No data available after preprocessing") + output_data = Tensor.empty(groups[0].shape) output_data = output_data.cat(*groups, dim=0)
147-155:⚠️ Potential issueFix incorrect tuple creation in get_preprocessors method.
The trailing commas on lines 149 and 155 create tuples instead of just returning the dictionaries.
Remove the trailing commas:
def get_preprocessors(self) -> Dict[str, Any]: means_by_ticker = ( - {ticker: values["means"] for ticker, values in self.scalers.items()}, + {ticker: values["means"] for ticker, values in self.scalers.items()} ) standard_deviations_by_ticker = ( { ticker: values["standard_deviations"] for ticker, values in self.scalers.items() - }, + } )
177-177:⚠️ Potential issueFix incorrect tensor operation - use
catinstead ofstack.
Tensor.stackcreates a new dimension, but here you want to concatenate along the existing dimension.- batch_data = Tensor(batch_data).stack(padding, dim=0) + batch_data = batch_data.cat(padding, dim=0)
164-193:⚠️ Potential issueFix column-index mapping and slicing in
batches.The code records
self.preprocessors["indices"]before the DataFrame is reshaped—addingtime_indexand reordering columns—so numeric indices no longer line up with the final tensor. This causes incorrect slicing.Please update:
- In
load_data: after building and sortingself.data, recompute indices:self.preprocessors["indices"] = { col: idx for idx, col in enumerate(self.data.columns) }- In the
batchesmethod: slice out exactly the numeric columns by the recomputed indices.Run the following script to verify the current column structure after preprocessing:
#!/bin/bash # Description: Verify the column structure and indices in the DataSet class # Search for where indices are set and data is manipulated rg -A 10 "self\.preprocessors\[\"indices\"\]" echo "---" # Look for where self.data is assigned to understand final structure rg -A 5 "self\.data ="application/predictionengine/src/predictionengine/main.py (3)
45-49:⚠️ Potential issueRemove unreachable code block.
The check for
response.status_code == 404is unreachable becauseraise_for_status()on line 43 will already raise an exception for 4xx status codes, including 404.response = requests.get(url, params=parameters, timeout=30) response.raise_for_status() - if response.status_code == 404: - raise HTTPException( - status_code=404, - detail="No historical data found for the specified date range", - ) -
136-147: 🛠️ Refactor suggestionSimplify the batch iteration logic.
Since the dataset is configured with
batch_size=1andsample_count=1, there will only be one batch. The nested loop structure is unnecessary and confusing.- for tickers_batch, features_batch, _ in dataset.batches(): - percentile_25, percentile_50, percentile_75 = model.predict( - tickers_batch, features_batch - ) - - predictions[ticker] = { - "percentile_25": float(percentile_25[0]), - "percentile_50": float(percentile_50[0]), - "percentile_75": float(percentile_75[0]), - } - break + # Get the single batch + batch_generator = dataset.batches() + tickers_batch, features_batch, _ = next(batch_generator) + + percentile_25, percentile_50, percentile_75 = model.predict( + tickers_batch, features_batch + ) + + predictions[ticker] = { + "percentile_25": float(percentile_25[0]), + "percentile_50": float(percentile_50[0]), + "percentile_75": float(percentile_75[0]), + }
160-160:⚠️ Potential issueUse exception chaining for better error context.
When re-raising exceptions, use
from eto preserve the original exception context.- raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") from e🧰 Tools
🪛 Ruff (0.11.9)
160-160: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (3)
Dockerfile.test (2)
9-9: Clarify comment terminology
Consider refining the comment for clarity and consistency:- # copied separately from volume definitions + # copied separately from volume mounts
10-10: Ensure multi-source COPY destination ends with a slash
Dockerfile best practices require that when copying multiple sources, the destination path ends with a slash. Update this line as follows:- COPY pyproject.toml uv.lock . + COPY pyproject.toml uv.lock ./🧰 Tools
🪛 Hadolint (2.12.0)
[error] 10-10: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
application/predictionengine/src/predictionengine/main.py (1)
51-51: Move import to the top of the file.Importing modules inside functions can impact performance and readability. Consider moving this import to the top of the file.
+import pyarrow as pa import os import traceback from datetime import date, datetime, timedeltaThen remove line 51:
- import pyarrow as pa -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/test.yaml(1 hunks).mise.toml(1 hunks)Dockerfile.test(1 hunks)application/datamanager/Dockerfile(1 hunks)application/datamanager/pyproject.toml(1 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/Dockerfile(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)application/positionmanager/src/positionmanager/main.py(2 hunks)application/predictionengine/Dockerfile(1 hunks)application/predictionengine/pyproject.toml(1 hunks)application/predictionengine/src/predictionengine/dataset.py(1 hunks)application/predictionengine/src/predictionengine/gated_residual_network.py(1 hunks)application/predictionengine/src/predictionengine/long_short_term_memory.py(1 hunks)application/predictionengine/src/predictionengine/loss_function.py(1 hunks)application/predictionengine/src/predictionengine/main.py(1 hunks)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py(1 hunks)application/predictionengine/src/predictionengine/models.py(1 hunks)application/predictionengine/src/predictionengine/multi_head_self_attention.py(1 hunks)application/predictionengine/src/predictionengine/post_processor.py(1 hunks)application/predictionengine/src/predictionengine/ticker_embedding.py(1 hunks)application/predictionengine/tests/test_dataset.py(1 hunks)application/predictionengine/tests/test_gated_residual_network.py(1 hunks)application/predictionengine/tests/test_long_short_term_memory.py(1 hunks)application/predictionengine/tests/test_loss_function.py(1 hunks)application/predictionengine/tests/test_multi_head_self_attention.py(1 hunks)application/predictionengine/tests/test_post_processor.py(1 hunks)application/predictionengine/tests/test_ticker_embedding.py(1 hunks)compose.yaml(1 hunks)pyproject.toml(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- application/datamanager/Dockerfile
- application/predictionengine/tests/test_dataset.py
- application/predictionengine/src/predictionengine/post_processor.py
🚧 Files skipped from review as they are similar to previous changes (23)
- application/positionmanager/Dockerfile
- application/datamanager/pyproject.toml
- application/predictionengine/src/predictionengine/models.py
- .github/workflows/test.yaml
- .mise.toml
- compose.yaml
- application/predictionengine/src/predictionengine/ticker_embedding.py
- application/predictionengine/pyproject.toml
- application/predictionengine/tests/test_ticker_embedding.py
- application/positionmanager/src/positionmanager/main.py
- application/datamanager/src/datamanager/config.py
- application/predictionengine/tests/test_loss_function.py
- application/predictionengine/tests/test_gated_residual_network.py
- application/positionmanager/src/positionmanager/clients.py
- pyproject.toml
- application/predictionengine/Dockerfile
- application/predictionengine/tests/test_multi_head_self_attention.py
- application/predictionengine/tests/test_long_short_term_memory.py
- application/predictionengine/tests/test_post_processor.py
- application/predictionengine/src/predictionengine/gated_residual_network.py
- application/predictionengine/src/predictionengine/long_short_term_memory.py
- application/predictionengine/src/predictionengine/multi_head_self_attention.py
- application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
application/predictionengine/src/predictionengine/main.py (3)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (3)
MiniatureTemporalFusionTransformer(23-190)load(169-174)predict(176-190)application/predictionengine/src/predictionengine/dataset.py (4)
DataSet(18-193)load_data(42-144)get_preprocessors(146-162)batches(164-193)application/predictionengine/src/predictionengine/models.py (1)
PredictionResponse(5-6)
🪛 Ruff (0.11.9)
application/predictionengine/src/predictionengine/main.py
160-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
application/predictionengine/src/predictionengine/loss_function.py
4-4: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🪛 Hadolint (2.12.0)
Dockerfile.test
[error] 10-10: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
🔇 Additional comments (3)
Dockerfile.test (3)
1-1: Verify the Python base image version against project requirements
The AI summary mentions updates for Python 3.12 compatibility, but this test Dockerfile still usespython:3.13. Confirm whether this is intentional or if it should be aligned with 3.12.Likely an incorrect or invalid review comment.
12-12: Verifyuv syncflag compatibility
You changed theuv synccommand from--all-groupsto--all-packages. Please confirm that the installed version ofuvsupports--all-packagesand that this change produces the expected behavior.
14-14: Pre-create coverage output directory
Creating/tests/coverage_outputwithmkdir -pensures consistency with CI and workflow changes. This addition looks good.
forstmeier
force-pushed
the
05-25-build_initial_prediction_engine_service
branch
from
07f5f0e to
0e586e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
application/predictionengine/src/predictionengine/loss_function.py (2)
4-4: Fix mutable default argument anti-pattern.Using a mutable list as a default argument can lead to unexpected behavior when the function is called multiple times, as the same list object is reused across calls.
🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
4-10: Add input validation and documentation.The function lacks input validation and documentation, which could lead to runtime errors or confusion about its purpose and usage.
🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
application/predictionengine/src/predictionengine/main.py (4)
45-49: Remove unreachable code block.The check for
response.status_code == 404is unreachable becauseraise_for_status()on line 43 will already raise an exception for 4xx status codes, including 404.
77-78: Fix incorrect preprocessor access.The
get_preprocessors()method returns dictionaries formeans_by_tickerandstandard_deviations_by_ticker, not lists or tuples. Remove the[0]indexing.
136-147: Simplify the batch iteration logic.Since the dataset is configured with
batch_size=1andsample_count=1, there will only be one batch. The nested loop structure is unnecessary and confusing.
160-160: Use exception chaining for better error context.When re-raising exceptions, use
from eto preserve the original exception context.🧰 Tools
🪛 Ruff (0.11.9)
160-160: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (2)
Dockerfile.test (2)
9-9: Enhance the comment for clarity
The comment# copied separately from volume definitionscould be more descriptive by mentioning the intent—e.g., leveraging Docker layer caching when only dependency files change.
14-14: Leverage WORKDIR for directory creation
SinceWORKDIR /testsis already set, you can simplify the path and avoid hardcoding/tests:-RUN mkdir -p /tests/coverage_output +RUN mkdir -p coverage_outputThis keeps the Dockerfile more maintainable if the working directory ever changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/test.yaml(1 hunks).mise.toml(1 hunks)Dockerfile.test(1 hunks)application/datamanager/Dockerfile(1 hunks)application/datamanager/pyproject.toml(1 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/Dockerfile(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)application/positionmanager/src/positionmanager/main.py(2 hunks)application/predictionengine/Dockerfile(1 hunks)application/predictionengine/pyproject.toml(1 hunks)application/predictionengine/src/predictionengine/dataset.py(1 hunks)application/predictionengine/src/predictionengine/gated_residual_network.py(1 hunks)application/predictionengine/src/predictionengine/long_short_term_memory.py(1 hunks)application/predictionengine/src/predictionengine/loss_function.py(1 hunks)application/predictionengine/src/predictionengine/main.py(1 hunks)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py(1 hunks)application/predictionengine/src/predictionengine/models.py(1 hunks)application/predictionengine/src/predictionengine/multi_head_self_attention.py(1 hunks)application/predictionengine/src/predictionengine/post_processor.py(1 hunks)application/predictionengine/src/predictionengine/ticker_embedding.py(1 hunks)application/predictionengine/tests/test_dataset.py(1 hunks)application/predictionengine/tests/test_gated_residual_network.py(1 hunks)application/predictionengine/tests/test_long_short_term_memory.py(1 hunks)application/predictionengine/tests/test_loss_function.py(1 hunks)application/predictionengine/tests/test_multi_head_self_attention.py(1 hunks)application/predictionengine/tests/test_post_processor.py(1 hunks)application/predictionengine/tests/test_ticker_embedding.py(1 hunks)compose.yaml(1 hunks)pyproject.toml(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/test.yaml
- application/predictionengine/src/predictionengine/post_processor.py
🚧 Files skipped from review as they are similar to previous changes (25)
- .mise.toml
- application/positionmanager/Dockerfile
- application/datamanager/Dockerfile
- application/datamanager/pyproject.toml
- application/positionmanager/src/positionmanager/main.py
- application/predictionengine/src/predictionengine/models.py
- application/predictionengine/src/predictionengine/ticker_embedding.py
- compose.yaml
- application/predictionengine/tests/test_ticker_embedding.py
- application/datamanager/src/datamanager/config.py
- application/predictionengine/pyproject.toml
- pyproject.toml
- application/positionmanager/src/positionmanager/clients.py
- application/predictionengine/tests/test_loss_function.py
- application/predictionengine/tests/test_gated_residual_network.py
- application/predictionengine/Dockerfile
- application/predictionengine/tests/test_dataset.py
- application/predictionengine/tests/test_long_short_term_memory.py
- application/predictionengine/src/predictionengine/long_short_term_memory.py
- application/predictionengine/tests/test_post_processor.py
- application/predictionengine/src/predictionengine/gated_residual_network.py
- application/predictionengine/tests/test_multi_head_self_attention.py
- application/predictionengine/src/predictionengine/multi_head_self_attention.py
- application/predictionengine/src/predictionengine/dataset.py
- application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
🧰 Additional context used
🪛 Hadolint (2.12.0)
Dockerfile.test
[error] 10-10: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
🪛 Ruff (0.11.9)
application/predictionengine/src/predictionengine/loss_function.py
4-4: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
application/predictionengine/src/predictionengine/main.py
160-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
Dockerfile.test (1)
12-12: Verify theuv syncflag change
The switch from--all-groupsto--all-packagesmay alter which dependencies are synced. Please confirm against theuvtool’s documentation or run a quick dry-run locally to ensure it still installs all required packages.
forstmeier
force-pushed
the
05-25-build_initial_prediction_engine_service
branch
from
0e586e7 to
39b2bed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
application/predictionengine/src/predictionengine/loss_function.py (2)
4-4: Fix mutable default argument anti-pattern.Using a mutable list as a default argument can lead to unexpected behavior when the function is called multiple times, as the same list object is reused across calls.
Apply this diff to fix the issue:
-def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]): +def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None): + if quantiles is None: + quantiles = [0.25, 0.5, 0.75]🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
4-10: Add input validation and documentation.The function lacks input validation and documentation, which could lead to runtime errors or confusion about its purpose and usage.
Consider adding input validation and comprehensive documentation:
+def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None): + """ + Compute quantile loss for predicted and true tensor values. + + Args: + y_pred: Predicted values tensor + y_true: True values tensor + quantiles: List of quantiles to compute loss for (default: [0.25, 0.5, 0.75]) + + Returns: + Tensor: Accumulated quantile loss + + Raises: + ValueError: If tensors have incompatible shapes or quantiles are invalid + """ + if quantiles is None: + quantiles = [0.25, 0.5, 0.75] + + if y_pred.shape != y_true.shape: + raise ValueError(f"Shape mismatch: y_pred {y_pred.shape} vs y_true {y_true.shape}") + + if not all(0 <= q <= 1 for q in quantiles): + raise ValueError("All quantiles must be between 0 and 1") + -def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]): loss = Tensor.zeros(1)🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
application/predictionengine/src/predictionengine/gated_residual_network.py (1)
11-13: Fix inconsistent None check for required parameter.The parameter
output_sizeis declared asint(required), but line 13 checks if it'sNone. This is inconsistent since a requiredintparameter cannot beNone.Either make the parameter optional:
- output_size: int, + output_size: int = None,Or remove the None check:
- output_size = output_size if output_size is not None else input_size + # output_size is always provided as per function signatureapplication/predictionengine/src/predictionengine/dataset.py (6)
23-28: Fix mutable default argument to avoid shared state issues.Using a mutable default argument (empty dict) can lead to unexpected behavior as the same dict object is shared across all instances. Note that line 28 checks for
Nonebut the default is{}.Apply this fix:
- scalers: Dict[str, Dict[str, Tensor]] = {}, + scalers: Dict[str, Dict[str, Tensor]] = None, ) -> None: self.batch_size = batch_size self.sequence_length = sequence_length self.sample_count = sample_count self.scalers = scalers if scalers is not None else {}🧰 Tools
🪛 Ruff (0.11.9)
23-23: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
34-140: Consider refactoring theload_datamethod for better maintainability.This method is doing too many things: type casting, date range generation, interpolation, encoding, scaling, and concatenation. Consider breaking it down into smaller, focused methods.
Example refactoring approach:
_cast_columns()- Handle type casting_generate_complete_time_series()- Create full date ranges_encode_tickers()- Handle ticker encoding_compute_scalers()- Calculate means and standard deviations_scale_data()- Apply scaling transformationsWould you like me to generate the refactored implementation?
110-111: Fix incorrect scalers check and remove redundant type annotation.The condition
self.scalers is Nonewill never be true sinceself.scalersis initialized in__init__. Also, re-declaring the instance variable with a type annotation is unnecessary and confusing.- if self.scalers is None or len(self.scalers) == 0: - self.scalers: Dict[str, Dict[str, Tensor]] = {} + if len(self.scalers) == 0:
136-137: Add error handling for empty groups list.If no data remains after grouping (e.g., all tickers are filtered out),
groups[0]will raise an IndexError.+ if not groups: + raise ValueError("No data available after preprocessing") + output_data = Tensor.empty(groups[0].shape) output_data = output_data.cat(*groups, dim=0)
173-173: Fix incorrect tensor operation - usecatinstead ofstack.
Tensor.stackcreates a new dimension, but here you want to concatenate along the existing dimension.- batch_data = Tensor(batch_data).stack(padding, dim=0) + batch_data = batch_data.cat(padding, dim=0)
160-189: Fix column-index mapping and slicing inbatchesmethod.The code uses
self.preprocessors["indices"]which was computed before DataFrame transformations. After addingtime_indexand reordering columns, the indices no longer align with the final tensor structure.This causes:
close_price_idxto reference the wrong columnhistorical_featuresto include incorrect columns- Feature count mismatch
Update the implementation:
- Ensure indices are recomputed after data transformations (see previous comment)
- In
batches, carefully select only the numeric feature columns forhistorical_featuresclose_price_idx = self.preprocessors["indices"]["close_price"] + # Ensure we're slicing the correct columns for features + # After transformations, column 0 is ticker, columns 1-6 are the continuous variables # ... in the loop ... - historical_features = Tensor.stack( - *[ - batch_data[i : i + self.sequence_length, 1:] - for i in range(self.batch_size) - ], - dim=0, - ) + # Extract only the 6 continuous variable columns (indices 1-6) + historical_features = Tensor.stack( + *[ + batch_data[i : i + self.sequence_length, 1:7] + for i in range(self.batch_size) + ], + dim=0, + )application/predictionengine/src/predictionengine/main.py (4)
45-49: Remove unreachable code block.The check for
response.status_code == 404is unreachable becauseraise_for_status()on line 43 will already raise an exception for 4xx status codes, including 404.
77-78: Fix incorrect preprocessor access.The
get_preprocessors()method returns dictionaries formeans_by_tickerandstandard_deviations_by_ticker, not lists or tuples. Remove the[0]indexing.
136-147: Simplify the batch iteration logic.Since the dataset is configured with
batch_size=1andsample_count=1, there will only be one batch. The nested loop structure is unnecessary and confusing.
160-160: Use exception chaining for better error context.When re-raising exceptions, use
from eto preserve the original exception context.🧰 Tools
🪛 Ruff (0.11.9)
160-160: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (2)
application/predictionengine/src/predictionengine/gated_residual_network.py (1)
45-46: Remove unnecessary type cast.The
castis redundant since tensor operations already returnTensorobjects.- gated_output = cast(Tensor, gate_state * output_state + residual) + gated_output = gate_state * output_state + residual return self.layer_normalizer(gated_output)application/predictionengine/src/predictionengine/main.py (1)
17-17: Consider improving environment variable handling.Using an empty string as the default for
DATAMANAGER_BASE_URLcould lead to invalid URLs if the environment variable is not set. Consider using a more robust approach:- datamanager_base_url = os.getenv("DATAMANAGER_BASE_URL", "") + datamanager_base_url = os.getenv("DATAMANAGER_BASE_URL") + if not datamanager_base_url: + raise ValueError("DATAMANAGER_BASE_URL environment variable must be set")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.dockerignore(1 hunks).github/workflows/test.yaml(1 hunks).mise.toml(1 hunks)Dockerfile.test(1 hunks)application/datamanager/Dockerfile(1 hunks)application/datamanager/pyproject.toml(1 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/Dockerfile(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)application/positionmanager/src/positionmanager/main.py(2 hunks)application/predictionengine/Dockerfile(1 hunks)application/predictionengine/pyproject.toml(1 hunks)application/predictionengine/src/predictionengine/dataset.py(1 hunks)application/predictionengine/src/predictionengine/gated_residual_network.py(1 hunks)application/predictionengine/src/predictionengine/long_short_term_memory.py(1 hunks)application/predictionengine/src/predictionengine/loss_function.py(1 hunks)application/predictionengine/src/predictionengine/main.py(1 hunks)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py(1 hunks)application/predictionengine/src/predictionengine/models.py(1 hunks)application/predictionengine/src/predictionengine/multi_head_self_attention.py(1 hunks)application/predictionengine/src/predictionengine/post_processor.py(1 hunks)application/predictionengine/src/predictionengine/ticker_embedding.py(1 hunks)application/predictionengine/tests/test_dataset.py(1 hunks)application/predictionengine/tests/test_gated_residual_network.py(1 hunks)application/predictionengine/tests/test_long_short_term_memory.py(1 hunks)application/predictionengine/tests/test_loss_function.py(1 hunks)application/predictionengine/tests/test_multi_head_self_attention.py(1 hunks)application/predictionengine/tests/test_post_processor.py(1 hunks)application/predictionengine/tests/test_ticker_embedding.py(1 hunks)compose.yaml(1 hunks)pyproject.toml(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/test.yaml
- application/datamanager/Dockerfile
- .dockerignore
- application/predictionengine/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (23)
- application/positionmanager/Dockerfile
- application/predictionengine/src/predictionengine/ticker_embedding.py
- application/datamanager/pyproject.toml
- application/positionmanager/src/positionmanager/main.py
- application/predictionengine/Dockerfile
- application/predictionengine/src/predictionengine/models.py
- .mise.toml
- application/predictionengine/tests/test_loss_function.py
- application/datamanager/src/datamanager/config.py
- application/predictionengine/tests/test_gated_residual_network.py
- Dockerfile.test
- application/positionmanager/src/positionmanager/clients.py
- compose.yaml
- application/predictionengine/tests/test_ticker_embedding.py
- application/predictionengine/tests/test_multi_head_self_attention.py
- application/predictionengine/tests/test_dataset.py
- application/predictionengine/src/predictionengine/multi_head_self_attention.py
- application/predictionengine/src/predictionengine/long_short_term_memory.py
- pyproject.toml
- application/predictionengine/src/predictionengine/post_processor.py
- application/predictionengine/tests/test_long_short_term_memory.py
- application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
- application/predictionengine/tests/test_post_processor.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
application/predictionengine/src/predictionengine/gated_residual_network.py (2)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (1)
forward(75-101)application/predictionengine/src/predictionengine/ticker_embedding.py (1)
forward(9-10)
🪛 Ruff (0.11.9)
application/predictionengine/src/predictionengine/dataset.py
23-23: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
application/predictionengine/src/predictionengine/loss_function.py
4-4: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
application/predictionengine/src/predictionengine/main.py
160-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (5)
application/predictionengine/src/predictionengine/main.py (5)
1-13: LGTM! Well-organized imports.The imports are properly structured with standard library, third-party, and local modules clearly separated.
24-25: LGTM! Proper FastAPI setup with monitoring.The application setup correctly includes Prometheus instrumentation for observability.
28-30: LGTM! Standard health check implementation.The health check endpoint follows best practices for service monitoring.
121-134: LGTM! Proper data validation and preparation.The logic for filtering ticker data, validating sufficient data points, and preparing datasets for prediction is well-implemented.
148-153: LGTM! Appropriate error handling for empty predictions.The validation that predictions were generated and the corresponding HTTP 404 response is appropriate.
| self.preprocessors["indices"] = { | ||
| col: idx for idx, col in enumerate(data.columns) | ||
| } |
There was a problem hiding this comment.
Move index computation after data transformations.
The column indices are captured too early, before the data undergoes significant transformations (joins, new columns, reordering). This causes the indices to become stale.
Move the index computation after the data is finalized:
- self.preprocessors["indices"] = {
- col: idx for idx, col in enumerate(data.columns)
- }
-
data = data.unique(subset=["ticker", "timestamp"])
# ... all transformations ...
data = data.sort(["ticker", "timestamp"])
+
+ # Compute indices after all transformations
+ self.preprocessors["indices"] = {
+ col: idx for idx, col in enumerate(data.columns)
+ }Also applies to: 84-85
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/dataset.py at lines 48-50
and also lines 84-85, the computation of column indices is done too early before
the data transformations like joins, adding new columns, or reordering. This
causes the stored indices to be outdated. To fix this, move the code that sets
self.preprocessors["indices"] to after all data transformations are complete,
ensuring the indices reflect the final state of the data columns.
forstmeier
force-pushed
the
05-25-build_initial_prediction_engine_service
branch
from
39b2bed to
3b76b59
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
application/predictionengine/src/predictionengine/loss_function.py (1)
4-4: Fix mutable default argument anti-pattern.Using a mutable list as a default argument can lead to unexpected behavior when the function is called multiple times, as the same list object is reused across calls.
Apply this diff to fix the issue:
-def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=[0.25, 0.5, 0.75]): +def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None): + if quantiles is None: + quantiles = [0.25, 0.5, 0.75]🧰 Tools
🪛 Ruff (0.11.9)
4-4: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
application/predictionengine/src/predictionengine/dataset.py (5)
23-23: Fix mutable default argument to avoid shared state issues.Using a mutable default argument (empty dict) can lead to unexpected behavior as the same dict object is shared across all instances.
Apply this fix:
- scalers: Dict[str, Dict[str, Tensor]] = {}, + scalers: Dict[str, Dict[str, Tensor]] = None,🧰 Tools
🪛 Ruff (0.11.9)
23-23: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
48-50: Move index computation after data transformations.The column indices are captured too early, before the data undergoes significant transformations (joins, new columns, reordering). This causes the indices to become stale.
Move the index computation after the data is finalized:
- self.preprocessors["indices"] = { - col: idx for idx, col in enumerate(data.columns) - } - data = data.unique(subset=["ticker", "timestamp"]) # ... all transformations ... data = data.sort(["ticker", "timestamp"]) + + # Compute indices after all transformations + self.preprocessors["indices"] = { + col: idx for idx, col in enumerate(data.columns) + }
110-111: Fix incorrect scalers check and remove redundant type annotation.The condition
self.scalers is Nonewill never be true sinceself.scalersis initialized as{}in__init__. Also, re-declaring the instance variable with a type annotation is unnecessary and confusing.- if self.scalers is None or len(self.scalers) == 0: - self.scalers: Dict[str, Dict[str, Tensor]] = {} + if len(self.scalers) == 0:
173-173: Fix incorrect tensor operation - usecatinstead ofstack.
Tensor.stackcreates a new dimension, but here you want to concatenate along the existing dimension.- batch_data = Tensor(batch_data).stack(padding, dim=0) + batch_data = batch_data.cat(padding, dim=0)
28-28:⚠️ Potential issueIncorrect fix for mutable default argument.
The current fix
scalers if scalers is not None else {}is checking against None but the default is still a mutable dict{}, not None.Apply this fix:
- scalers: Dict[str, Dict[str, Tensor]] = {}, + scalers: Dict[str, Dict[str, Tensor]] = None, ) -> None: self.batch_size = batch_size self.sequence_length = sequence_length self.sample_count = sample_count self.scalers = scalers if scalers is not None else {}application/predictionengine/src/predictionengine/main.py (4)
45-49: Remove unreachable code block.The check for
response.status_code == 404is unreachable becauseraise_for_status()on line 43 will already raise an exception for 4xx status codes, including 404.response = requests.get(url, params=parameters, timeout=30) response.raise_for_status() - if response.status_code == 404: - raise HTTPException( - status_code=404, - detail="No historical data found for the specified date range", - ) -
77-78: Fix incorrect preprocessor access.The
get_preprocessors()method returns dictionaries formeans_by_tickerandstandard_deviations_by_ticker, not lists or tuples. Remove the[0]indexing.- means_by_ticker=preprocessors["means_by_ticker"][0], - standard_deviations_by_ticker=preprocessors["standard_deviations_by_ticker"][0], + means_by_ticker=preprocessors["means_by_ticker"], + standard_deviations_by_ticker=preprocessors["standard_deviations_by_ticker"],
136-147: Simplify the batch iteration logic.Since the dataset is configured with
batch_size=1andsample_count=1, there will only be one batch. The nested loop structure is unnecessary and confusing.- for tickers_batch, features_batch, _ in dataset.batches(): - percentile_25, percentile_50, percentile_75 = model.predict( - tickers_batch, features_batch - ) - - predictions[ticker] = { - "percentile_25": float(percentile_25[0]), - "percentile_50": float(percentile_50[0]), - "percentile_75": float(percentile_75[0]), - } - break + # Get the single batch + batch_generator = dataset.batches() + tickers_batch, features_batch, _ = next(batch_generator) + + percentile_25, percentile_50, percentile_75 = model.predict( + tickers_batch, features_batch + ) + + predictions[ticker] = { + "percentile_25": float(percentile_25[0]), + "percentile_50": float(percentile_50[0]), + "percentile_75": float(percentile_75[0]), + }
160-160: Use exception chaining for better error context.When re-raising exceptions, use
from eto preserve the original exception context.- raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") from e🧰 Tools
🪛 Ruff (0.11.9)
160-160: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (2)
117-134: Fix the incorrect batch iteration logic and typo.The nested loop structure is incorrect.
dataset.batches()returns individual batches, not a collection to iterate over. Also, there's a typo in the variable name and the loss calculation should be per epoch, not per batch.- for batch in dataset.batches(): - for tickers, historical_features, targets in batch: - predictions, _, _ = self.forward( - Tensor(tickers), - Tensor(historical_features), - ) - - loss = quantile_loss(predictions, Tensor(targets), quantiles) - - optimizer.zero_grad() - loss.backward() - optimizer.step() - - epoch_loss += loss.numpy().item() - - avgerage_epoch_loss = epoch_loss / len(dataset) - losses.append(avgerage_epoch_loss) + batch_count = 0 + for tickers, historical_features, targets in dataset.batches(): + predictions, _, _ = self.forward( + Tensor(tickers), + Tensor(historical_features), + ) + + loss = quantile_loss(predictions, Tensor(targets), quantiles) + + optimizer.zero_grad() + loss.backward() + optimizer.step() + + epoch_loss += loss.numpy().item() + batch_count += 1 + + average_epoch_loss = epoch_loss / batch_count + losses.append(average_epoch_loss)
144-150: Fix the batch iteration in validation method.The batch iteration has the same issue as in the train method. Also,
len(dataset)might not return the correct number of batches.def validate( self, dataset: DataSet, ) -> float: total_loss = 0.0 - batch_count = len(dataset) + batch_count = 0 - for batch in dataset.batches(): - tickers, features, targets = batch + for tickers, features, targets in dataset.batches(): tickers, features, targets = ( Tensor(tickers), Tensor(features), Tensor(targets), ) + batch_count += 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.dockerignore(1 hunks).github/workflows/test.yaml(1 hunks).mise.toml(2 hunks)Dockerfile.test(1 hunks)application/datamanager/Dockerfile(1 hunks)application/datamanager/pyproject.toml(1 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/Dockerfile(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)application/positionmanager/src/positionmanager/main.py(2 hunks)application/predictionengine/Dockerfile(1 hunks)application/predictionengine/pyproject.toml(1 hunks)application/predictionengine/src/predictionengine/dataset.py(1 hunks)application/predictionengine/src/predictionengine/gated_residual_network.py(1 hunks)application/predictionengine/src/predictionengine/long_short_term_memory.py(1 hunks)application/predictionengine/src/predictionengine/loss_function.py(1 hunks)application/predictionengine/src/predictionengine/main.py(1 hunks)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py(1 hunks)application/predictionengine/src/predictionengine/models.py(1 hunks)application/predictionengine/src/predictionengine/multi_head_self_attention.py(1 hunks)application/predictionengine/src/predictionengine/post_processor.py(1 hunks)application/predictionengine/src/predictionengine/ticker_embedding.py(1 hunks)application/predictionengine/tests/test_dataset.py(1 hunks)application/predictionengine/tests/test_gated_residual_network.py(1 hunks)application/predictionengine/tests/test_long_short_term_memory.py(1 hunks)application/predictionengine/tests/test_loss_function.py(1 hunks)application/predictionengine/tests/test_multi_head_self_attention.py(1 hunks)application/predictionengine/tests/test_post_processor.py(1 hunks)application/predictionengine/tests/test_ticker_embedding.py(1 hunks)compose.yaml(1 hunks)pyproject.toml(3 hunks)
✅ Files skipped from review due to trivial changes (5)
- .github/workflows/test.yaml
- application/datamanager/Dockerfile
- application/predictionengine/tests/test_ticker_embedding.py
- application/predictionengine/pyproject.toml
- application/predictionengine/src/predictionengine/post_processor.py
🚧 Files skipped from review as they are similar to previous changes (22)
- application/positionmanager/Dockerfile
- .dockerignore
- application/datamanager/pyproject.toml
- application/datamanager/src/datamanager/config.py
- application/predictionengine/src/predictionengine/ticker_embedding.py
- compose.yaml
- application/positionmanager/src/positionmanager/main.py
- .mise.toml
- application/predictionengine/tests/test_multi_head_self_attention.py
- application/predictionengine/tests/test_long_short_term_memory.py
- application/predictionengine/tests/test_gated_residual_network.py
- application/positionmanager/src/positionmanager/clients.py
- Dockerfile.test
- application/predictionengine/src/predictionengine/gated_residual_network.py
- application/predictionengine/src/predictionengine/models.py
- application/predictionengine/tests/test_loss_function.py
- pyproject.toml
- application/predictionengine/tests/test_dataset.py
- application/predictionengine/src/predictionengine/multi_head_self_attention.py
- application/predictionengine/src/predictionengine/long_short_term_memory.py
- application/predictionengine/tests/test_post_processor.py
- application/predictionengine/Dockerfile
🧰 Additional context used
🧬 Code Graph Analysis (2)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (7)
application/predictionengine/src/predictionengine/ticker_embedding.py (2)
TickerEmbedding(5-10)forward(9-10)application/predictionengine/src/predictionengine/long_short_term_memory.py (2)
LongShortTermMemory(6-62)forward(23-62)application/predictionengine/src/predictionengine/gated_residual_network.py (2)
GatedResidualNetwork(6-46)forward(30-46)application/predictionengine/src/predictionengine/multi_head_self_attention.py (2)
MultiHeadSelfAttention(7-63)forward(29-63)application/predictionengine/src/predictionengine/post_processor.py (2)
PostProcessor(8-47)post_process_predictions(19-47)application/predictionengine/src/predictionengine/dataset.py (1)
batches(160-189)application/predictionengine/src/predictionengine/loss_function.py (1)
quantile_loss(4-10)
application/predictionengine/src/predictionengine/main.py (3)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (3)
MiniatureTemporalFusionTransformer(23-190)load(169-174)predict(176-190)application/predictionengine/src/predictionengine/dataset.py (4)
DataSet(17-189)load_data(34-139)get_preprocessors(141-158)batches(160-189)application/predictionengine/src/predictionengine/models.py (1)
PredictionResponse(5-6)
🪛 Ruff (0.11.9)
application/predictionengine/src/predictionengine/dataset.py
23-23: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
application/predictionengine/src/predictionengine/loss_function.py
4-4: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
application/predictionengine/src/predictionengine/main.py
160-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
…n.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…or.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…elf_attention.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
application/predictionengine/src/predictionengine/loss_function.py (2)
4-6: Mutable default argument issue has been resolved.The previous issue with mutable default arguments has been properly fixed by using
Noneas default and initializing the list inside the function.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 4-4: Missing function or method docstring
(C0116)
4-13: 🛠️ Refactor suggestionAdd comprehensive documentation and input validation.
The function still lacks proper documentation and input validation as suggested in previous reviews. This is critical for a loss function that will be used in model training.
def quantile_loss(y_pred: Tensor, y_true: Tensor, quantiles=None): + """ + Compute quantile loss for predicted and true tensor values. + + Args: + y_pred: Predicted values tensor + y_true: True values tensor + quantiles: List of quantiles to compute loss for (default: [0.25, 0.5, 0.75]) + + Returns: + Tensor: Accumulated quantile loss + + Raises: + ValueError: If tensors have incompatible shapes or quantiles are invalid + """ if quantiles is None: quantiles = [0.25, 0.5, 0.75] - # …rest of implementation… + + if y_pred.shape != y_true.shape: + raise ValueError(f"Shape mismatch: y_pred {y_pred.shape} vs y_true {y_true.shape}") + + if not all(0 <= q <= 1 for q in quantiles): + raise ValueError("All quantiles must be between 0 and 1") + loss = Tensor.zeros(1)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 4-4: Missing function or method docstring
(C0116)
application/predictionengine/src/predictionengine/main.py (1)
133-143: Simplify the batch iteration logic.Since the dataset is configured with
batch_size=1andsample_count=1, there will only be one batch. The nested loop structure is unnecessary and confusing.
🧹 Nitpick comments (5)
application/predictionengine/src/predictionengine/loss_function.py (1)
1-1: Add module docstring.The module lacks a docstring explaining its purpose and the quantile loss implementation.
+""" +Quantile loss function for deep learning models. + +This module provides quantile loss calculation for training models +that predict multiple quantiles simultaneously. +""" from tinygrad.tensor import Tensor🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 1-1: Unable to import 'tinygrad.tensor'
(E0401)
application/predictionengine/src/predictionengine/main.py (4)
48-48: Move pyarrow import to module level.Importing modules inside functions can impact performance and readability. Move this import to the top of the file.
+import pyarrow as pa import os import traceback from datetime import date, datetime, timedelta-import pyarrow as pa - buffer = pa.py_buffer(response.content)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 48-48: Import outside toplevel (pyarrow)
(C0415)
[error] 48-48: Unable to import 'pyarrow'
(E0401)
15-21: Add docstring for lifespan function.The async context manager lacks documentation explaining its purpose in managing application lifecycle.
@asynccontextmanager async def lifespan(app: FastAPI): + """ + Manage FastAPI application lifecycle. + + Sets up application state including datamanager URL and model placeholder + during startup, and handles cleanup during shutdown. + """ datamanager_base_url = os.getenv("DATAMANAGER_BASE_URL", "")🧰 Tools
🪛 Pylint (3.3.7)
[convention] 16-16: Missing function or method docstring
(C0116)
33-54: Add comprehensive documentation and error handling.The function lacks proper documentation and could benefit from more specific error handling.
def fetch_historical_data( datamanager_url: str, start_date: date, end_date: date ) -> pl.DataFrame: + """ + Fetch historical equity data from the data manager service. + + Args: + datamanager_url: Base URL of the data manager service + start_date: Start date for data retrieval + end_date: End date for data retrieval + + Returns: + DataFrame containing historical equity bar data + + Raises: + HTTPException: If the data manager service is unreachable or returns an error + ValueError: If the response cannot be parsed as Arrow data + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 33-33: Missing function or method docstring
(C0116)
[convention] 48-48: Import outside toplevel (pyarrow)
(C0415)
[error] 48-48: Unable to import 'pyarrow'
(E0401)
91-150: Consider breaking down the prediction endpoint.The endpoint function has many responsibilities and local variables. Consider extracting helper functions for data preparation, model initialization, and prediction generation to improve maintainability.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 92-92: Missing function or method docstring
(C0116)
[refactor] 92-92: Too many local variables (17/15)
(R0914)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
application/predictionengine/src/predictionengine/loss_function.py(1 hunks)application/predictionengine/src/predictionengine/main.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
application/predictionengine/src/predictionengine/loss_function.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 1-1: Unable to import 'tinygrad.tensor'
(E0401)
[convention] 4-4: Missing function or method docstring
(C0116)
application/predictionengine/src/predictionengine/main.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'polars'
(E0401)
[error] 7-7: Unable to import 'fastapi'
(E0401)
[error] 8-8: Unable to import 'prometheus_fastapi_instrumentator'
(E0401)
[error] 9-9: Unable to import 'loguru'
(E0401)
[error] 10-10: Attempted relative import beyond top-level package
(E0402)
[error] 11-11: Attempted relative import beyond top-level package
(E0402)
[error] 12-12: Attempted relative import beyond top-level package
(E0402)
[convention] 16-16: Missing function or method docstring
(C0116)
[convention] 29-29: Missing function or method docstring
(C0116)
[convention] 33-33: Missing function or method docstring
(C0116)
[convention] 48-48: Import outside toplevel (pyarrow)
(C0415)
[error] 48-48: Unable to import 'pyarrow'
(E0401)
[convention] 57-57: Missing function or method docstring
(C0116)
[warning] 85-85: Catching too general exception Exception
(W0718)
[convention] 92-92: Missing function or method docstring
(C0116)
[refactor] 92-92: Too many local variables (17/15)
(R0914)
🔇 Additional comments (2)
application/predictionengine/src/predictionengine/main.py (2)
157-157: Exception chaining has been properly implemented.Good fix! The exception chaining with
from epreserves the original exception context.
74-76: Preprocessor access has been corrected.The incorrect
[0]indexing has been removed and the dictionaries are now accessed correctly.
| response = requests.get(url, params=parameters, timeout=30) | ||
| response.raise_for_status() | ||
|
|
||
| response = requests.get(url, params=parameters, timeout=30) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
Remove duplicate HTTP request.
There are duplicate HTTP requests being made, which is inefficient and could cause unexpected behavior.
response = requests.get(url, params=parameters, timeout=30)
response.raise_for_status()
-response = requests.get(url, params=parameters, timeout=30)
-response.raise_for_status()
-
import pyarrow as pa📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response = requests.get(url, params=parameters, timeout=30) | |
| response.raise_for_status() | |
| response = requests.get(url, params=parameters, timeout=30) | |
| response.raise_for_status() | |
| response = requests.get(url, params=parameters, timeout=30) | |
| response.raise_for_status() | |
| import pyarrow as pa |
🤖 Prompt for AI Agents
In application/predictionengine/src/predictionengine/main.py around lines 42 to
46, there are duplicate HTTP GET requests to the same URL with the same
parameters and timeout. Remove the redundant second request and its
raise_for_status call to avoid unnecessary network calls and potential side
effects.
…mporal_fusion_transformer.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
application/datamanager/src/datamanager/config.py (1)
8-8: Consider validation for required configuration fields.While defaulting to empty strings provides type consistency, it may mask configuration errors. Consider adding validation to ensure critical fields like API keys and project identifiers are properly configured.
The past review comment about adding field validators for these critical configuration fields is still applicable and should be addressed.
Also applies to: 14-15
application/predictionengine/src/predictionengine/multi_head_self_attention.py (1)
52-52: Optimize tensor handling by removing unnecessary cast.The
cast()call is unnecessary sinceattention_scoresshould already be a Tensor. This aligns with the previous feedback about avoiding tensor wrapping.- attention_weights: Tensor = cast(Tensor, attention_scores).softmax(axis=-1) + attention_weights: Tensor = attention_scores.softmax(axis=-1)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (3)
121-138: Fix incorrect batch iteration logic and typo.The nested loop structure is incorrect and there's a typo in the variable name. This duplicates a previous review comment but the issue remains unresolved.
- for batch in dataset.batches(): - for tickers, historical_features, targets in batch: - predictions, _, _ = self.forward( - Tensor(tickers), - Tensor(historical_features), - ) - - loss = quantile_loss(predictions, Tensor(targets), quantiles) - - optimizer.zero_grad() - loss.backward() - optimizer.step() - - epoch_loss += loss.numpy().item() - - avgerage_epoch_loss = epoch_loss / len(dataset) - losses.append(avgerage_epoch_loss) + batch_count = 0 + for tickers, historical_features, targets in dataset.batches(): + predictions, _, _ = self.forward( + Tensor(tickers), + Tensor(historical_features), + ) + + loss = quantile_loss(predictions, Tensor(targets), quantiles) + + optimizer.zero_grad() + loss.backward() + optimizer.step() + + epoch_loss += loss.numpy().item() + batch_count += 1 + + average_epoch_loss = epoch_loss / batch_count + losses.append(average_epoch_loss)
146-154: Fix the batch iteration in validation method.The batch iteration has the same issue as in the train method. This duplicates a previous review comment but the issue remains unresolved.
def validate( self, dataset: DataSet, ) -> float: total_loss = 0.0 - batch_count = len(dataset) + batch_count = 0 - for batch in dataset.batches(): - tickers, features, targets = batch + for tickers, features, targets in dataset.batches(): tickers, features, targets = ( Tensor(tickers), Tensor(features), Tensor(targets), ) + batch_count += 1
88-94: Address shape mismatch between LSTM output and self-attention input.Based on the
LongShortTermMemoryimplementation in the codebase, it returns output with shape(batch_size, sequence_length, hidden_size), which should be compatible with the self-attention layer. However, the feature processor expects different dimensions.lstm_output, _ = self.lstm_encoder.forward(lstm_input) - processed_features = self.feature_processor.forward(lstm_output) + # LSTM output shape: (batch_size, seq_len, hidden_size) + # Take the last timestep for feature processing + last_hidden = lstm_output[:, -1, :] # (batch_size, hidden_size) + processed_features = self.feature_processor.forward(last_hidden) + + # Expand back to sequence format for attention + processed_features = processed_features.unsqueeze(1).expand(-1, lstm_output.shape[1], -1) attention_output, attention_weights = self.self_attention.forward( processed_features )
🧹 Nitpick comments (6)
workflows/prediction_model.py (1)
45-45: Add missing docstring for the workflow function.The function is missing a docstring to describe its purpose and parameters.
Apply this diff to add a docstring:
@workflow def training_workflow(start_date: datetime, end_date: datetime) -> None: + """Train and store a model using data from the specified date range.""" data = fetch_data(start_date=start_date, end_date=end_date)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 45-45: Missing function or method docstring
(C0116)
[refactor] 45-45: Useless return at end of function or method
(R1711)
application/predictionengine/src/predictionengine/post_processor.py (3)
8-17: Add class docstring for better documentation.The PostProcessor class would benefit from a docstring explaining its purpose and how it fits into the prediction pipeline.
Apply this diff to add a class docstring:
class PostProcessor: + """ + Post-processes prediction outputs by rescaling normalized predictions + back to original scale and computing quantile forecasts. + """ def __init__(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 8-8: Missing class docstring
(C0115)
[refactor] 8-8: Too few public methods (1/2)
(R0903)
19-27: Add method docstring for post_process_predictions.The method would benefit from documentation explaining its parameters and return values.
Apply this diff to add a method docstring:
def post_process_predictions( self, encoded_tickers: np.ndarray, predictions: np.ndarray, ) -> Tuple[ np.ndarray[Any, np.dtype[np.float64]], np.ndarray[Any, np.dtype[np.float64]], np.ndarray[Any, np.dtype[np.float64]], ]: + """ + Post-process predictions by rescaling and computing quantiles. + + Args: + encoded_tickers: Array of encoded ticker values + predictions: Normalized prediction values + + Returns: + Tuple of (25th percentile, 50th percentile, 75th percentile) arrays + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 19-19: Missing function or method docstring
(C0116)
[error] 24-24: Value 'np.ndarray' is unsubscriptable
(E1136)
[error] 25-25: Value 'np.ndarray' is unsubscriptable
(E1136)
[error] 26-26: Value 'np.ndarray' is unsubscriptable
(E1136)
39-39: Consider breaking the long line for better readability.The condition check exceeds the 100-character line limit.
Apply this diff to improve readability:
- if ticker not in self.means_by_ticker or ticker not in self.standard_deviations_by_ticker: + if (ticker not in self.means_by_ticker or + ticker not in self.standard_deviations_by_ticker):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 39-39: Line too long (102/100)
(C0301)
application/predictionengine/src/predictionengine/multi_head_self_attention.py (1)
7-7: Add class docstring for better documentation.Consider adding a docstring to document the class purpose, parameters, and usage.
class MultiHeadSelfAttention: + """Multi-head self-attention mechanism for sequence modeling. + + Args: + heads_count: Number of attention heads + embedding_size: Dimension of input embeddings (must be divisible by heads_count) + """ def __init__(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 7-7: Missing class docstring
(C0115)
[refactor] 7-7: Too many instance attributes (8/7)
(R0902)
[refactor] 7-7: Too few public methods (1/2)
(R0903)
todos.md (1)
92-92: Fix missing language specification in code block.-``` +```python🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
92-92: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.gitignore(1 hunks).mise.toml(2 hunks)application/datamanager/pyproject.toml(1 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)application/positionmanager/src/positionmanager/main.py(2 hunks)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py(1 hunks)application/predictionengine/src/predictionengine/multi_head_self_attention.py(1 hunks)application/predictionengine/src/predictionengine/post_processor.py(1 hunks)infrastructure/cloud_run.py(1 hunks)pyproject.toml(3 hunks)todos.md(1 hunks)workflows/prediction_model.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- infrastructure/cloud_run.py
🚧 Files skipped from review as they are similar to previous changes (5)
- application/datamanager/pyproject.toml
- application/positionmanager/src/positionmanager/main.py
- application/positionmanager/src/positionmanager/clients.py
- .mise.toml
- pyproject.toml
🧰 Additional context used
🧬 Code Graph Analysis (1)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (7)
application/predictionengine/src/predictionengine/ticker_embedding.py (2)
TickerEmbedding(5-10)forward(9-10)application/predictionengine/src/predictionengine/long_short_term_memory.py (2)
LongShortTermMemory(6-62)forward(23-62)application/predictionengine/src/predictionengine/gated_residual_network.py (2)
GatedResidualNetwork(6-46)forward(30-46)application/predictionengine/src/predictionengine/multi_head_self_attention.py (2)
MultiHeadSelfAttention(7-62)forward(28-62)application/predictionengine/src/predictionengine/post_processor.py (2)
PostProcessor(8-48)post_process_predictions(19-48)application/predictionengine/src/predictionengine/dataset.py (1)
batches(160-189)application/predictionengine/src/predictionengine/loss_function.py (1)
quantile_loss(4-13)
🪛 Pylint (3.3.7)
workflows/prediction_model.py
[convention] 45-45: Missing function or method docstring
(C0116)
[refactor] 45-45: Useless return at end of function or method
(R1711)
[warning] 48-48: Unused variable 'artifact_path'
(W0612)
application/predictionengine/src/predictionengine/multi_head_self_attention.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 2-2: Unable to import 'tinygrad.tensor'
(E0401)
[error] 3-3: Unable to import 'tinygrad.nn'
(E0401)
[error] 4-4: Unable to import 'tinygrad.dtype'
(E0401)
[convention] 7-7: Missing class docstring
(C0115)
[refactor] 7-7: Too many instance attributes (8/7)
(R0902)
[convention] 28-28: Missing function or method docstring
(C0116)
[warning] 30-30: Redefining built-in 'input'
(W0622)
[refactor] 7-7: Too few public methods (1/2)
(R0903)
application/datamanager/src/datamanager/config.py
[convention] 13-13: Missing class docstring
(C0115)
[refactor] 13-13: Too few public methods (1/2)
(R0903)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 2-2: Unable to import 'category_encoders'
(E0401)
[error] 3-3: Attempted relative import beyond top-level package
(E0402)
[error] 4-4: Attempted relative import beyond top-level package
(E0402)
[error] 5-5: Attempted relative import beyond top-level package
(E0402)
[error] 6-6: Attempted relative import beyond top-level package
(E0402)
[error] 7-7: Attempted relative import beyond top-level package
(E0402)
[error] 8-8: Unable to import 'tinygrad.tensor'
(E0401)
[error] 9-9: Unable to import 'tinygrad.nn.optim'
(E0401)
[error] 10-16: Unable to import 'tinygrad.nn.state'
(E0401)
[error] 18-18: Unable to import 'numpy'
(E0401)
[error] 19-19: Attempted relative import beyond top-level package
(E0402)
[error] 20-20: Attempted relative import beyond top-level package
(E0402)
[convention] 23-23: Missing class docstring
(C0115)
[refactor] 24-24: Too many arguments (12/5)
(R0913)
[refactor] 24-24: Too many positional arguments (12/5)
(R0917)
[convention] 75-75: Missing function or method docstring
(C0116)
[error] 86-86: Undefined variable 'torch'
(E0602)
[convention] 107-107: Missing function or method docstring
(C0116)
[convention] 141-141: Missing function or method docstring
(C0116)
[convention] 166-166: Missing function or method docstring
(C0116)
[convention] 173-173: Missing function or method docstring
(C0116)
[convention] 180-180: Missing function or method docstring
(C0116)
[warning] 183-183: Redefining built-in 'input'
(W0622)
[convention] 8-8: third party import "tinygrad.tensor.Tensor" should be placed before local imports "ticker_embedding.TickerEmbedding", "long_short_term_memory.LongShortTermMemory", "gated_residual_network.GatedResidualNetwork", "multi_head_self_attention.MultiHeadSelfAttention", "post_processor.PostProcessor"
(C0411)
[convention] 9-9: third party import "tinygrad.nn.optim.Adam" should be placed before local imports "ticker_embedding.TickerEmbedding", "long_short_term_memory.LongShortTermMemory", "gated_residual_network.GatedResidualNetwork", "multi_head_self_attention.MultiHeadSelfAttention", "post_processor.PostProcessor"
(C0411)
[convention] 10-16: third party import "tinygrad.nn.state.get_parameters" should be placed before local imports "ticker_embedding.TickerEmbedding", "long_short_term_memory.LongShortTermMemory", "gated_residual_network.GatedResidualNetwork", "multi_head_self_attention.MultiHeadSelfAttention", "post_processor.PostProcessor"
(C0411)
[convention] 17-17: standard import "typing.Tuple" should be placed before third party imports "category_encoders.OrdinalEncoder", "tinygrad.tensor.Tensor", "tinygrad.nn.optim.Adam", "tinygrad.nn.state.get_parameters" and local imports "ticker_embedding.TickerEmbedding", "long_short_term_memory.LongShortTermMemory", "gated_residual_network.GatedResidualNetwork", "multi_head_self_attention.MultiHeadSelfAttention", "post_processor.PostProcessor"
(C0411)
[convention] 18-18: third party import "numpy" should be placed before local imports "ticker_embedding.TickerEmbedding", "long_short_term_memory.LongShortTermMemory", "gated_residual_network.GatedResidualNetwork", "multi_head_self_attention.MultiHeadSelfAttention", "post_processor.PostProcessor"
(C0411)
application/predictionengine/src/predictionengine/post_processor.py
[convention] 39-39: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 2-2: Unable to import 'tinygrad.tensor'
(E0401)
[error] 3-3: Unable to import 'category_encoders'
(E0401)
[error] 4-4: Unable to import 'numpy'
(E0401)
[error] 5-5: Unable to import 'polars'
(E0401)
[convention] 8-8: Missing class docstring
(C0115)
[convention] 19-19: Missing function or method docstring
(C0116)
[error] 24-24: Value 'np.ndarray' is unsubscriptable
(E1136)
[error] 25-25: Value 'np.ndarray' is unsubscriptable
(E1136)
[error] 26-26: Value 'np.ndarray' is unsubscriptable
(E1136)
[refactor] 8-8: Too few public methods (1/2)
(R0903)
🪛 LanguageTool
todos.md
[uncategorized] ~66-~66: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...scalers check and remove redundant type annotation. The condition self.scalers is None wi...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~68-~68: Possible missing comma found.
Context: ...tion self.scalers is None will never be true since self.scalers is initialized as {}...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~231-~231: You might be missing the article “the” here.
Context: ...loss) ``` # Fix the batch iteration in validation method. The batch iteration has the sa...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~233-~233: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... the same issue as in the train method. Also, len(dataset) might not return the corr...
(REP_SENT_START_ALSO)
🪛 markdownlint-cli2 (0.17.2)
todos.md
1-1: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
36-36: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
66-66: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
82-82: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
92-92: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
104-104: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
164-164: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
189-189: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
231-231: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
🔇 Additional comments (4)
.gitignore (1)
14-14: LGTM! Good addition to ignore todos.md.Adding
todos.mdto.gitignoreis appropriate for keeping development notes out of version control.application/predictionengine/src/predictionengine/post_processor.py (1)
39-40: Error handling for missing ticker statistics is well implemented.The error handling for missing ticker statistics that was suggested in previous reviews has been properly implemented. This prevents KeyError exceptions when tickers are missing from the statistics dictionaries.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 39-39: Line too long (102/100)
(C0301)
todos.md (2)
1-35: Excellent configuration validation recommendations.The suggestions for adding Pydantic field validators to ensure critical configuration fields are non-empty are spot-on. This will prevent silent misconfigurations that could lead to runtime failures.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Trailing punctuation in heading
Punctuation: '.'(MD026, no-trailing-punctuation)
160-187: Critical shape mismatch issue correctly identified.The analysis of the LSTM output vs self-attention input shape mismatch is accurate and provides clear solutions. This is a fundamental issue that would cause runtime errors.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
164-164: Spaces inside emphasis markers
null(MD037, no-space-in-emphasis)
| .coverage* | ||
| .coverage/ | ||
| coverage.xml No newline at end of file | ||
| coverage.xmlcoverage.xml |
There was a problem hiding this comment.
Fix the duplicated coverage.xml entry.
The line appears to have a concatenation error with coverage.xmlcoverage.xml. This should be just coverage.xml.
Apply this diff to fix the typo:
-coverage.xmlcoverage.xml
+coverage.xml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| coverage.xmlcoverage.xml | |
| coverage.xml |
🤖 Prompt for AI Agents
In the .gitignore file at line 12, there is a duplicated entry
"coverage.xmlcoverage.xml" due to a concatenation error. Replace this line with
a single "coverage.xml" entry to correctly ignore the coverage.xml file.
| artifact_path = store_model(model_bytes=model_bytes) | ||
| return artifact_path | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable and unnecessary return statement.
Since the workflow no longer returns the artifact path, the artifact_path variable is unused and the explicit return statement is unnecessary.
Apply this diff to clean up the code:
@workflow
def training_workflow(start_date: datetime, end_date: datetime) -> None:
data = fetch_data(start_date=start_date, end_date=end_date)
model_bytes = train_dummy_model(data=data)
- artifact_path = store_model(model_bytes=model_bytes)
- return
+ store_model(model_bytes=model_bytes)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| artifact_path = store_model(model_bytes=model_bytes) | |
| return artifact_path | |
| return | |
| @workflow | |
| def training_workflow(start_date: datetime, end_date: datetime) -> None: | |
| data = fetch_data(start_date=start_date, end_date=end_date) | |
| model_bytes = train_dummy_model(data=data) | |
| store_model(model_bytes=model_bytes) |
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 48-48: Unused variable 'artifact_path'
(W0612)
🤖 Prompt for AI Agents
In workflows/prediction_model.py around lines 48 to 49, remove the unused
variable assignment to artifact_path and delete the unnecessary explicit return
statement since the function no longer returns a value. Simply call
store_model(model_bytes=model_bytes) without assigning its result, and omit the
return statement.
| self.lstm_encoder = LongShortTermMemory( | ||
| input_size=input_size, | ||
| hidden_size=hidden_size + embedding_size, | ||
| layer_count=layer_count, | ||
| dropout_rate=dropout_rate, | ||
| ) |
There was a problem hiding this comment.
Fix LSTM input size calculation.
The LSTM is initialized with input_size + embedding_size as hidden size, but it should receive input_size + embedding_size as input size since ticker embeddings are concatenated to features.
self.lstm_encoder = LongShortTermMemory(
- input_size=input_size,
- hidden_size=hidden_size + embedding_size,
+ input_size=input_size + embedding_size,
+ hidden_size=hidden_size,
layer_count=layer_count,
dropout_rate=dropout_rate,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.lstm_encoder = LongShortTermMemory( | |
| input_size=input_size, | |
| hidden_size=hidden_size + embedding_size, | |
| layer_count=layer_count, | |
| dropout_rate=dropout_rate, | |
| ) | |
| self.lstm_encoder = LongShortTermMemory( | |
| input_size=input_size + embedding_size, | |
| hidden_size=hidden_size, | |
| layer_count=layer_count, | |
| dropout_rate=dropout_rate, | |
| ) |
🤖 Prompt for AI Agents
In
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
around lines 43 to 48, the LSTM is incorrectly initialized with hidden_size set
to hidden_size + embedding_size instead of adjusting the input_size. Fix this by
changing the LSTM initialization to have input_size equal to input_size +
embedding_size, and keep hidden_size as hidden_size. This correctly reflects
that ticker embeddings are concatenated to the input features.
| ticker_embeddings = self.ticker_embedding.forward(tickers) | ||
|
|
||
| # get ticker embeddings and expand to (batch_size, seq_len, embedding_dim) | ||
| ticker_embeddings = self.ticker_embedding.forward(tickers) # (batch_size, embedding_dim) | ||
| ticker_embeddings = ticker_embeddings.unsqueeze(1).expand(-1, features.size(1), -1) | ||
| # concatenate along the feature dimension | ||
| lstm_input = torch.cat([features, ticker_embeddings], dim=-1) |
There was a problem hiding this comment.
Fix duplicate ticker embedding computation and missing torch import.
There are several critical issues in this code:
- Ticker embeddings are computed twice (lines 80 and 83)
torchis used but not imported- The tinygrad framework is being used, so tensor operations should use tinygrad APIs
- ticker_embeddings = self.ticker_embedding.forward(tickers)
-
- # get ticker embeddings and expand to (batch_size, seq_len, embedding_dim)
- ticker_embeddings = self.ticker_embedding.forward(tickers) # (batch_size, embedding_dim)
- ticker_embeddings = ticker_embeddings.unsqueeze(1).expand(-1, features.size(1), -1)
- # concatenate along the feature dimension
- lstm_input = torch.cat([features, ticker_embeddings], dim=-1)
+ # get ticker embeddings and expand to (batch_size, seq_len, embedding_dim)
+ ticker_embeddings = self.ticker_embedding.forward(tickers) # (batch_size, embedding_dim)
+ ticker_embeddings = ticker_embeddings.unsqueeze(1).expand(-1, features.shape[1], -1)
+ # concatenate along the feature dimension
+ lstm_input = features.cat(ticker_embeddings, dim=-1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ticker_embeddings = self.ticker_embedding.forward(tickers) | |
| # get ticker embeddings and expand to (batch_size, seq_len, embedding_dim) | |
| ticker_embeddings = self.ticker_embedding.forward(tickers) # (batch_size, embedding_dim) | |
| ticker_embeddings = ticker_embeddings.unsqueeze(1).expand(-1, features.size(1), -1) | |
| # concatenate along the feature dimension | |
| lstm_input = torch.cat([features, ticker_embeddings], dim=-1) | |
| # get ticker embeddings and expand to (batch_size, seq_len, embedding_dim) | |
| ticker_embeddings = self.ticker_embedding.forward(tickers) # (batch_size, embedding_dim) | |
| ticker_embeddings = ticker_embeddings.unsqueeze(1).expand(-1, features.shape[1], -1) | |
| # concatenate along the feature dimension | |
| lstm_input = features.cat(ticker_embeddings, dim=-1) |
🧰 Tools
🪛 Pylint (3.3.7)
[error] 86-86: Undefined variable 'torch'
(E0602)
🤖 Prompt for AI Agents
In
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
around lines 80 to 86, remove the duplicate call to
self.ticker_embedding.forward(tickers) so ticker embeddings are computed only
once. Replace the use of torch functions like unsqueeze and expand with their
tinygrad equivalents, and ensure tinygrad is properly imported at the top of the
file instead of torch. This will fix redundant computation, missing import, and
framework API usage issues.
Merge pull request #576 from pocketsizefund/fixes Fixes fixes
chrisaddy
force-pushed
the
05-25-build_initial_prediction_engine_service
branch
from
956e4df to
1c3ed7e
Compare
Overview
Changes
predictionengineservice intinygradComments
This model has not been tested and still needs to be incorporated into the Flyte pipeline @chrisaddy.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.dockerignoreto exclude virtual environments and cache files from Docker builds.Tests