Conversation
infra updates
|
Warning Rate limit exceeded@chrisaddy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
""" WalkthroughThis update restructures infrastructure provisioning by consolidating Cloud Run, Pub/Sub, and Cloud Scheduler resource creation into Changes
Sequence Diagram(s)sequenceDiagram
participant Pulumi as Pulumi
participant EnvMod as environment_variables.py
participant SvcMod as services.py
participant Main as __main__.py
participant GCP as GCP Cloud
Main->>EnvMod: Import environment variable helpers/constants
Main->>SvcMod: Import create_service
Main->>GCP: Create datamanager Cloud Run service (with env vars)
Main->>GCP: Create predictionengine Cloud Run service (with DATAMANAGER_BASE_URL)
Main->>GCP: Create positionmanager Cloud Run service (with API keys, DATAMANAGER_BASE_URL)
Main->>GCP: Create Pub/Sub subscription (pushes to datamanager)
Main->>GCP: Create Cloud Scheduler job (publishes to Pub/Sub topic)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
application/datamanager/pyproject.toml (1)
12-16:⚠️ Potential issueRemove duplicated dependency entry.
Theloguru>=0.7.3package appears twice in the dependencies list (lines 12 and 16). Consolidate into a single entry to avoid redundancy.
🧹 Nitpick comments (9)
application/datamanager/src/datamanager/main.py (1)
42-42: LGTM! Type annotation improvement.The updated return type annotation
AsyncGenerator[None, None]correctly specifies both the yield type and return type, making the async context manager's behavior more explicit and improving type safety.Consider adding a docstring to document the lifespan function's purpose:
@asynccontextmanager +async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: + """Manage application lifespan: initialize settings, storage client, and DuckDB connection.""" -async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 42-42: Missing function or method docstring
(C0116)
application/predictionengine/src/predictionengine/main.py (1)
17-17: LGTM! Consistent type annotation improvement.The updated return type annotation
AsyncGenerator[None, None]aligns with the same change in the datamanager service, ensuring consistency across the codebase while improving type clarity.Consider adding a docstring to document the lifespan function's purpose:
@asynccontextmanager +async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: + """Manage application lifespan: initialize datamanager base URL and model state.""" -async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 17-17: Missing function or method docstring
(C0116)
infrastructure/services.py (1)
20-24: Add function docstring for better documentation.The function lacks documentation explaining its purpose, parameters, and return value.
Add a comprehensive docstring:
def create_service( name: str, envs: list[ENVIRONMENT_VARIABLE] | None = None ) -> Service: + """Create and deploy a Cloud Run service. + + Args: + name: The service name, used for Docker image tagging and service naming + envs: Optional list of environment variables for the service + + Returns: + A configured Cloud Run Service resource + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 20-20: Missing function or method docstring
(C0116)
application/predictionengine/compose.yaml (1)
14-14: Consider security implications of mounting credentials.Mounting Google Cloud credentials directly from the host may not be the most secure approach for integration testing.
Consider using service account key files or environment variables instead:
- - ~/.config/gcloud/application_default_credentials.json:/root/.config/gcloud/application_default_credentials.json:ro + # Alternative: Use GOOGLE_APPLICATION_CREDENTIALS environment variable + # - GOOGLE_APPLICATION_CREDENTIALS=/path/to/service-account-key.jsoninfrastructure/environment_variables.py (3)
1-3: Add module docstring for better documentation.Consider adding a module docstring to describe the purpose of this module and its role in the infrastructure setup.
+""" +Environment variable definitions for Pulumi GCP Cloud Run services. + +This module centralizes the creation of environment variables used across +multiple Cloud Run services, securely handling secret configuration values. +""" from pulumi import Output from pulumi.config import Config from pulumi_gcp.cloudrun import ServiceTemplateSpecContainerEnvArgs🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 1-1: Unable to import 'pulumi'
(E0401)
[error] 2-2: Unable to import 'pulumi.config'
(E0401)
[error] 3-3: Unable to import 'pulumi_gcp.cloudrun'
(E0401)
7-7: Consider removing unnecessary type alias.The type alias
ENVIRONMENT_VARIABLEdoesn't add much value and could be simplified by using the original type directly.-ENVIRONMENT_VARIABLE = ServiceTemplateSpecContainerEnvArgs - - def create_environment_variable( name: str, value: str | Output[str] -) -> ENVIRONMENT_VARIABLE: +) -> ServiceTemplateSpecContainerEnvArgs: return ServiceTemplateSpecContainerEnvArgs(name=name, value=value)
10-13: Add function docstring for better documentation.The helper function would benefit from a docstring explaining its purpose and parameters.
def create_environment_variable( name: str, value: str | Output[str] ) -> ServiceTemplateSpecContainerEnvArgs: + """Create a Cloud Run environment variable. + + Args: + name: The environment variable name + value: The environment variable value (string or Pulumi Output) + + Returns: + ServiceTemplateSpecContainerEnvArgs: Configured environment variable + """ return ServiceTemplateSpecContainerEnvArgs(name=name, value=value)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 10-10: Missing function or method docstring
(C0116)
infrastructure/__main__.py (2)
1-1: Add module docstring for better documentation.Consider adding a module docstring to describe the main infrastructure setup and its components.
+""" +Main infrastructure setup for Cloud Run services and supporting resources. + +This module creates and configures: +- Cloud Run services (datamanager, predictionengine, positionmanager) +- Pub/Sub subscription for service communication +- Cloud Scheduler job for periodic triggers +""" import base64🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
47-48: Clarify commented environment variables.The commented environment variables for portfolio tickers should either be implemented or removed. If they're planned for future implementation, consider adding a TODO comment explaining when they'll be needed.
DATAMANAGER_BASE_URL, - # MINIMUM_PORTFOLIO_TICKERS, # 20 - # MAXIMUM_PORTFOLIO_TICKERS, # 20 + # TODO: Add portfolio ticker limits when configuration is available + # MINIMUM_PORTFOLIO_TICKERS, + # MAXIMUM_PORTFOLIO_TICKERS,
📜 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 (12)
application/datamanager/pyproject.toml(1 hunks)application/datamanager/src/datamanager/main.py(1 hunks)application/positionmanager/pyproject.toml(1 hunks)application/predictionengine/compose.yaml(1 hunks)application/predictionengine/pyproject.toml(1 hunks)application/predictionengine/src/predictionengine/main.py(1 hunks)infrastructure/__main__.py(1 hunks)infrastructure/cloud_run.py(0 hunks)infrastructure/environment_variables.py(1 hunks)infrastructure/pyproject.toml(1 hunks)infrastructure/services.py(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- infrastructure/cloud_run.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
application/predictionengine/src/predictionengine/main.py (1)
application/datamanager/src/datamanager/main.py (1)
lifespan(42-64)
application/datamanager/src/datamanager/main.py (1)
application/predictionengine/src/predictionengine/main.py (1)
lifespan(17-22)
🪛 Pylint (3.3.7)
application/predictionengine/src/predictionengine/main.py
[convention] 17-17: Missing function or method docstring
(C0116)
application/datamanager/src/datamanager/main.py
[convention] 42-42: Missing function or method docstring
(C0116)
infrastructure/services.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'pulumi_docker_build'
(E0401)
[error] 7-7: Unable to import 'pulumi.config'
(E0401)
[error] 8-15: Unable to import 'pulumi_gcp.cloudrun'
(E0401)
[convention] 20-20: Missing function or method docstring
(C0116)
[warning] 31-31: Unused variable 'image'
(W0612)
infrastructure/environment_variables.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 1-1: Unable to import 'pulumi'
(E0401)
[error] 2-2: Unable to import 'pulumi.config'
(E0401)
[error] 3-3: Unable to import 'pulumi_gcp.cloudrun'
(E0401)
[convention] 10-10: Missing function or method docstring
(C0116)
infrastructure/__main__.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 2-2: Unable to import 'pulumi_gcp'
(E0401)
[warning] 16-16: Unused import buckets
(W0611)
🔇 Additional comments (9)
infrastructure/pyproject.toml (1)
3-3: Approve version bump to date-based scheme.
Updating the project version to "20250602.4" aligns with the coordinated release cycle and matches the root project’s versioning strategy.pyproject.toml (1)
3-3: Approve root project version bump.
The root workspace version has been updated to "20250602.4" to stay in sync with the infrastructure and application components.application/positionmanager/pyproject.toml (1)
16-16: Approve addition of Prometheus FastAPI Instrumentator.
Addingprometheus-fastapi-instrumentator>=7.1.0will enable metrics instrumentation in the Position Manager service, improving observability.application/datamanager/pyproject.toml (1)
25-30: Approve addition ofdevdependency group.
Introducing adevgroup withbehave>=1.2.6supports behavior-driven tests for the Data Manager service.application/predictionengine/pyproject.toml (1)
13-14: Approve additions of Prometheus Instrumentator and Loguru.
The new dependenciesprometheus-fastapi-instrumentator>=7.1.0andloguru>=0.7.3bolster metrics collection and structured logging in the Prediction Engine service.infrastructure/services.py (2)
31-47: The image variable is correctly used despite static analysis warning.The static analysis tool incorrectly flags the
imagevariable as unused. While it's not directly referenced in subsequent code, it's essential for the Docker build process and the built image is referenced by tag in line 57.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 31-31: Unused variable 'image'
(W0612)
59-67: Review startup probe configuration for production readiness.The startup probe configuration has very generous timeouts (60s initial delay, 60s period, 50 failure threshold = up to 50 minutes startup time). Consider if these values are appropriate for your deployment requirements.
Could you verify if these startup probe values align with your expected service startup times? The current configuration allows up to 50 minutes for startup, which may be excessive for typical FastAPI applications.
infrastructure/environment_variables.py (1)
16-38: LGTM! Environment variable definitions are well-structured.The environment variable definitions follow a consistent pattern and properly use
config.require_secretfor secure handling of sensitive values. The centralized approach improves maintainability.infrastructure/__main__.py (1)
53-72: LGTM! Pub/Sub and Scheduler configuration is well-structured.The Pub/Sub subscription and Cloud Scheduler job are properly configured with:
- Correct OIDC authentication using platform service account
- Appropriate schedule (daily at midnight UTC)
- Proper base64 encoding of the JSON payload
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>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
application/predictionengine/compose.yaml(1 hunks)infrastructure/__main__.py(1 hunks)infrastructure/services.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- application/predictionengine/compose.yaml
🧰 Additional context used
🧬 Code Graph Analysis (1)
infrastructure/__main__.py (2)
infrastructure/environment_variables.py (1)
create_environment_variable(10-13)infrastructure/services.py (1)
create_service(20-79)
🪛 Ruff (0.11.9)
infrastructure/__main__.py
39-39: SyntaxError: Unexpected indentation
39-39: SyntaxError: Invalid assignment target
40-40: SyntaxError: Expected a statement
40-41: SyntaxError: Expected a statement
🪛 Pylint (3.3.7)
infrastructure/__main__.py
[error] 39-39: Parsing failed: 'unexpected indent (main, line 39)'
(E0001)
infrastructure/services.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'pulumi_docker_build'
(E0401)
[error] 7-7: Unable to import 'pulumi.config'
(E0401)
[error] 8-15: Unable to import 'pulumi_gcp.cloudrun'
(E0401)
[convention] 20-20: Missing function or method docstring
(C0116)
[warning] 38-38: Unused variable 'image'
(W0612)
🪛 GitHub Actions: Code quality check
infrastructure/__main__.py
[error] 39-39: Failed to parse file due to unexpected indentation at line 39.
🔇 Additional comments (3)
infrastructure/services.py (1)
26-36: Excellent error handling and validation improvements.The implementation now properly handles file reading errors and validates service directory existence, addressing the previous review concerns effectively.
infrastructure/__main__.py (2)
16-16: Good improvement to the import comment.The updated comment clearly explains why the
bucketsimport is necessary for Pulumi resource registration side effects.
19-30: Well-structured service creation with proper environment variables.The consolidated approach using
create_serviceimproves maintainability and consistency across service deployments. The environment variable configuration appropriately separates concerns between services.Also applies to: 43-52
forstmeier
left a comment
forstmeier
left a comment
There was a problem hiding this comment.
CodeRabbit has a few comments worth addressing and I have one nitpick question but it's not a big one.
…orage-access-issue Grant storage access
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/__main__.py (1)
47-48: Clarify the status of commented environment variables.The commented environment variables
MINIMUM_PORTFOLIO_TICKERSandMAXIMUM_PORTFOLIO_TICKERSshould either be implemented or removed to maintain code clarity.Based on the past review comment asking "Remove or keep?", please clarify:
- Are these variables needed for the positionmanager service functionality?
- If needed, what are the intended values (the comments suggest 20)?
- If not needed, they should be removed to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/__main__.py(1 hunks)infrastructure/buckets.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
infrastructure/__main__.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 2-2: Unable to import 'pulumi_gcp'
(E0401)
[warning] 16-16: Unused import buckets
(W0611)
🔇 Additional comments (6)
infrastructure/buckets.py (1)
15-22: LGTM! IAM binding correctly configured.The IAM member binding is properly implemented:
- Uses the appropriate
roles/storage.objectCreatorrole for write access- Correctly formats the service account email with the
serviceAccount:prefix- Follows Pulumi resource naming conventions
- Integrates well with the existing infrastructure
infrastructure/__main__.py (5)
19-30: LGTM! Service configuration is well-structured.The datamanager service is properly configured with all necessary environment variables for API keys, project settings, and data access.
32-34: Good pattern for inter-service communication.Creating the DATAMANAGER_BASE_URL environment variable from the service URL enables proper service-to-service communication configuration.
36-38: LGTM! Service name typo has been corrected.The variable name is now correctly spelled as "predictionengine_service" and the service configuration looks good.
53-62: LGTM! Pub/Sub subscription correctly configured.The subscription setup is well-implemented:
- Proper OIDC token authentication using the platform service account
- Correct push endpoint configuration targeting the datamanager service
- Follows security best practices for service authentication
64-72: LGTM! Cloud Scheduler job properly configured.The scheduler job is correctly set up:
- Appropriate cron schedule for daily execution at midnight UTC
- Proper base64 encoding of empty JSON payload
- Correct integration with the Pub/Sub topic
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Grant storage access PR commit fixes
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/__main__.py(1 hunks)infrastructure/ping.nu(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/main.py
🔇 Additional comments (1)
infrastructure/ping.nu (1)
1-1: LGTM: Proper authentication setup.The authorization header setup using
gcloud auth print-identity-tokenis the correct approach for authenticating with Google Cloud Run services.
fix bandit issues fixing ruff issues
update to fix blind exceptions
add bugbear add comma linting fix bugbear and commas add timezones
no fixme/todos implicit string concatenation import conventions logging pathlib
sort imports fix naming conventions
infra updates
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
infrastructure/__main__.py (1)
15-15: Remove unused noqa directive.The static analysis correctly identifies that this
noqadirective is no longer needed.-import topics # noqa: F401 +import topics🧰 Tools
🪛 Ruff (0.11.9)
15-15: Unused
noqadirective (unused:F401)Remove unused
noqadirective(RUF100)
📜 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 (19)
application/datamanager/features/environment.py(1 hunks)application/datamanager/features/steps/equity_bars_steps.py(4 hunks)application/datamanager/features/steps/health_steps.py(1 hunks)application/datamanager/src/datamanager/config.py(2 hunks)application/datamanager/src/datamanager/main.py(6 hunks)application/datamanager/src/datamanager/models.py(2 hunks)application/positionmanager/src/positionmanager/clients.py(6 hunks)application/positionmanager/src/positionmanager/main.py(8 hunks)application/positionmanager/src/positionmanager/models.py(3 hunks)application/positionmanager/src/positionmanager/portfolio.py(3 hunks)application/positionmanager/tests/test_positionmanager_main.py(7 hunks)infrastructure/__main__.py(1 hunks)infrastructure/buckets.py(2 hunks)infrastructure/images.py(2 hunks)infrastructure/monitoring.py(4 hunks)infrastructure/project.py(2 hunks)infrastructure/pyproject.toml(1 hunks)pyproject.toml(2 hunks)workflows/backfill_datamanager.py(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- application/datamanager/features/environment.py
- infrastructure/monitoring.py
- application/datamanager/features/steps/equity_bars_steps.py
- infrastructure/project.py
- workflows/backfill_datamanager.py
- application/positionmanager/src/positionmanager/portfolio.py
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/buckets.py
- infrastructure/pyproject.toml
🧰 Additional context used
🧬 Code Graph Analysis (4)
application/datamanager/features/steps/health_steps.py (1)
application/datamanager/features/steps/equity_bars_steps.py (1)
step_impl(50-54)
application/positionmanager/src/positionmanager/clients.py (1)
application/positionmanager/src/positionmanager/models.py (3)
DateRange(39-61)Money(9-36)to_payload(57-61)
application/datamanager/src/datamanager/main.py (3)
application/predictionengine/src/predictionengine/main.py (1)
lifespan(17-22)application/datamanager/src/datamanager/config.py (2)
Settings(44-46)daily_bars_path(20-23)application/datamanager/src/datamanager/models.py (2)
SummaryDate(7-28)BarsSummary(49-51)
infrastructure/__main__.py (2)
infrastructure/environment_variables.py (1)
create_environment_variable(10-13)infrastructure/services.py (1)
create_service(20-79)
🪛 GitHub Actions: Code quality check
application/datamanager/src/datamanager/config.py
[error] 22-22: TRY003 Avoid specifying long messages outside the exception class
🪛 Ruff (0.11.9)
application/positionmanager/src/positionmanager/main.py
6-6: typing.Dict is deprecated, use dict instead
(UP035)
6-6: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
6-6: Redefinition of unused Any from line 3
Remove definition: Any
(F811)
14-14: Redefinition of unused Instrumentator from line 10
Remove definition: Instrumentator
(F811)
17-17: Redefinition of unused AlpacaClient from line 8
Remove definition: AlpacaClient
(F811)
17-17: Redefinition of unused DataClient from line 8
Remove definition: DataClient
(F811)
18-18: Redefinition of unused DateRange from line 7
Remove definition: DateRange
(F811)
18-18: Redefinition of unused Money from line 7
Remove definition: Money
(F811)
18-18: Redefinition of unused PredictionPayload from line 7
Remove definition: PredictionPayload
(F811)
19-19: Redefinition of unused PortfolioOptimizer from line 9
Remove definition: PortfolioOptimizer
(F811)
50-50: Undefined name requests
(F821)
64-64: Undefined name requests
(F821)
77-77: Undefined name requests
(F821)
118-118: Undefined name requests
(F821)
152-152: Undefined name requests
(F821)
infrastructure/__main__.py
15-15: Unused noqa directive (unused: F401)
Remove unused noqa directive
(RUF100)
🪛 GitHub Check: Run Python quality checks
application/positionmanager/src/positionmanager/main.py
[failure] 6-6: Ruff (F811)
application/positionmanager/src/positionmanager/main.py:6:26: F811 Redefinition of unused Any from line 3
[failure] 6-6: Ruff (F401)
application/positionmanager/src/positionmanager/main.py:6:20: F401 typing.Dict imported but unused
[failure] 6-6: Ruff (UP035)
application/positionmanager/src/positionmanager/main.py:6:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 1-19: Ruff (I001)
application/positionmanager/src/positionmanager/main.py:1:1: I001 Import block is un-sorted or un-formatted
🪛 Pylint (3.3.7)
application/positionmanager/src/positionmanager/main.py
[refactor] 33-33: Too many local variables (16/15)
(R0914)
[error] 50-50: Undefined variable 'requests'
(E0602)
[error] 64-64: Undefined variable 'requests'
(E0602)
[error] 77-77: Undefined variable 'requests'
(E0602)
[error] 118-118: Undefined variable 'requests'
(E0602)
[error] 152-152: Undefined variable 'requests'
(E0602)
application/datamanager/src/datamanager/main.py
[error] 15-15: No name 'api_core' in module 'google'
(E0611)
[error] 16-16: No name 'api_core' in module 'google'
(E0611)
[error] 17-17: No name 'storage' in module 'google.cloud'
(E0611)
application/datamanager/src/datamanager/models.py
[error] 13-13: Method 'parse_date' should have "self" as first argument
(E0213)
[refactor] 7-7: Too few public methods (1/2)
(R0903)
application/positionmanager/src/positionmanager/models.py
[error] 16-16: Method 'validate_amount' should have "self" as first argument
(E0213)
[refactor] 64-64: Too few public methods (0/2)
(R0903)
🔇 Additional comments (40)
application/datamanager/features/steps/health_steps.py (2)
1-1: Good improvement: Import reorganization follows Python conventions.Moving the
requestsimport to the top follows PEP 8 guidelines for import ordering (third-party imports before local imports).
9-9: Excellent addition: HTTP request timeout prevents hanging requests.Adding the 30-second timeout improves robustness by preventing requests from hanging indefinitely. This aligns with the similar timeout additions in
equity_bars_steps.py.application/datamanager/src/datamanager/config.py (2)
2-4: Good improvement: Import reorganization and pathlib adoption.The import reorganization follows alphabetical ordering conventions, and adding
Pathfrompathlibsupports the modernization seen below.
32-33: Excellent modernization: Using pathlib for file operations.Replacing the built-in
open()function withPath().open()is a modern Python best practice that provides better path handling and more readable code. The direct return also eliminates an unnecessary intermediate variable.application/datamanager/src/datamanager/models.py (5)
9-9: Excellent improvement: Explicit timezone handling.Replacing
datetime.datetime.utcnow()withdatetime.datetime.now(tz=datetime.UTC)follows modern best practices by using timezone-aware datetime objects, preventing potential timezone-related bugs.
18-22: Good improvement: Explicit timezone setting in date parsing.Adding
.replace(tzinfo=datetime.UTC)ensures the parsed datetime is timezone-aware before converting to a date, maintaining consistency with the default factory change.
25-26: Good improvement: Error message variable assignment.Moving the error message to a variable before raising the exception follows the TRY003 linting rule and improves code maintainability.
37-40: Good improvement: Method signature formatting.The multi-line parameter formatting improves readability for methods with multiple parameters.
44-45: Good improvement: Consistent error message handling.Moving the error message to a variable before raising maintains consistency with the pattern established above and follows linting best practices.
pyproject.toml (2)
3-3: Version update follows date-based scheme.The version bump to "20250602.4" appears to follow a YYYYMMDD.patch format, which provides clear release tracking.
81-141: Excellent addition: Comprehensive linting configuration.The extensive ruff linting configuration enables many valuable code quality checks covering security, performance, maintainability, and style. The per-file ignores appropriately disable security assertion checks (S101) for test files where they're expected.
infrastructure/images.py (4)
1-2: LGTM: Improved imports for better functionality.Good additions of
UTCfor timezone-aware timestamps andpathlib.Pathfor better path handling.
13-16: LGTM: Dynamic path discovery improves maintainability.Using
pathlibandglob()to dynamically discover Dockerfile paths is much better than hardcoded paths. This makes the code more maintainable and flexible.
18-21: LGTM: Timezone-aware timestamps improve reliability.Using
datetime.now(tz=UTC)ensures consistent timestamps regardless of the system timezone.
51-51: LGTM: Proper logging replaces print statements.Using
logger.info()instead ofprint()provides better control over logging levels and output formatting.application/positionmanager/tests/test_positionmanager_main.py (3)
3-8: LGTM: Improved imports and FastAPI integration.Good reorganization of imports and addition of FastAPI status constants for better test readability.
22-22: LGTM: Using FastAPI status constants improves readability.Using
status.HTTP_200_OKinstead of hardcoded200makes the tests more maintainable and self-documenting.
87-90: LGTM: Proper exception mocking with HTTPException.The mock side effect now uses
HTTPExceptionwith proper status codes and details, which better simulates the actual FastAPI error handling.application/positionmanager/src/positionmanager/clients.py (4)
1-9: LGTM: Improved import organization and type hints.Good cleanup of imports and use of modern type hints with
Anyfrom typing.
20-21: LGTM: Better error message handling.Assigning error messages to variables before raising exceptions improves code readability and debugging.
38-38: LGTM: Modern type hints.Updated return type from
Dict[str, Any]todict[str, Any]follows modern Python best practices (PEP 585).
82-82: LGTM: Improved HTTP error handling.Using
response.raise_for_status()is better than manual status code checking and provides consistent error handling.application/positionmanager/src/positionmanager/main.py (3)
33-33: LGTM: Modern type hints.Updated return type from
Dict[str, Any]todict[str, Any]follows modern Python best practices.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 33-33: Too many local variables (16/15)
(R0914)
50-54: LGTM: Improved exception handling and error messages.Catching specific exceptions (
requests.RequestException,APIError,ValidationError) instead of genericExceptionis much better. The use of{e!r}provides detailed error representation.🧰 Tools
🪛 Ruff (0.11.9)
50-50: Undefined name
requests(F821)
🪛 Pylint (3.3.7)
[error] 50-50: Undefined variable 'requests'
(E0602)
57-58: LGTM: Timezone-aware datetime usage.Using
datetime.now(tz=UTC)ensures consistent behavior regardless of system timezone settings.application/positionmanager/src/positionmanager/models.py (4)
3-3: LGTM! Good addition of Any import for type hints.This import supports the updated type annotations throughout the models.
16-16: Type hint modernization looks good.The updated type hints using union syntax (
str | Decimal) and the# noqa: N805comment are appropriate. The static analysis warning about "self" is a false positive since this is a@field_validatorclassmethod whereclsis the correct first parameter.🧰 Tools
🪛 Pylint (3.3.7)
[error] 16-16: Method 'validate_amount' should have "self" as first argument
(E0213)
35-35: Excellent modernization of type hints.Replacing
Dictwith built-indictfollows Python 3.9+ best practices and improves code readability.Also applies to: 57-57, 65-65
52-53: Improved error handling pattern.Assigning the error message to a variable before raising the exception improves readability and follows Python best practices.
infrastructure/__main__.py (4)
1-14: Excellent modular infrastructure setup.The new import structure cleanly separates concerns with dedicated modules for environment variables and service creation. This improves maintainability and reusability.
19-30: Well-structured datamanager service configuration.The service creation with comprehensive environment variables properly supports all the required integrations (Alpaca, DuckDB, Polygon).
32-34: Smart dependency injection pattern.Creating the
DATAMANAGER_BASE_URLenvironment variable from the service URL and injecting it into dependent services establishes proper service-to-service communication.
51-70: Robust orchestration setup.The Pub/Sub subscription with OIDC authentication and Cloud Scheduler job creates a secure, automated workflow for triggering the datamanager service daily. The base64 encoding of the empty JSON payload is appropriate for Pub/Sub messaging.
application/datamanager/src/datamanager/main.py (7)
3-3: Excellent import additions for better exception handling.Adding specific exception types from
requests,duckdb,google.api_core, andpolarsenables more precise error handling throughout the application.Also applies to: 10-13, 16-16, 19-19
29-29: Appropriate security annotation for SQL query.The
# noqa: S608comment correctly suppresses the security warning for the parameterized SQL query, which is safe since the parameters are validated date objects.Also applies to: 46-46
50-50: Consistent async generator type annotation.The updated return type
AsyncGenerator[None, None]matches the pattern used inapplication/predictionengine/src/predictionengine/main.pyand is more precise than the previous annotation.
56-57: Appropriate naming convention suppression.The
# noqa: N806comments are correct for environment variable names that conventionally use uppercase.
105-106: Improved PyArrow usage and variable organization.Using the
paalias consistently and defining thecontent_dispositionvariable before use improves code readability.Also applies to: 109-111
123-129: Much improved exception handling specificity.Replacing generic
Exceptioncatching with specific exception types (requests.RequestException,ComputeError,IOException,GoogleAPIError,pyarrow.lib.ArrowIOError) improves debugging and prevents masking unexpected errors.Also applies to: 168-174
180-180: Good exception chaining practice.Using
from ein the HTTPException properly chains the original exception for better error traceability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
application/positionmanager/src/positionmanager/main.py (1)
28-133: Consider refactoring for better maintainability.The
create_positionfunction has multiple responsibilities and could benefit from being broken down into smaller, more focused functions. Consider extracting:
- Client initialization logic
- Portfolio optimization workflow
- Trade execution logic
- Result compilation
This would improve readability, testability, and maintainability.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 28-28: Too many local variables (16/15)
(R0914)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.flox/env/manifest.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
application/datamanager/src/datamanager/config.py(3 hunks)application/positionmanager/src/positionmanager/clients.py(4 hunks)application/positionmanager/src/positionmanager/main.py(8 hunks)application/predictionengine/src/predictionengine/dataset.py(8 hunks)application/predictionengine/src/predictionengine/gated_residual_network.py(2 hunks)application/predictionengine/src/predictionengine/long_short_term_memory.py(4 hunks)application/predictionengine/src/predictionengine/loss_function.py(2 hunks)application/predictionengine/src/predictionengine/main.py(7 hunks)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py(6 hunks)application/predictionengine/src/predictionengine/models.py(1 hunks)application/predictionengine/src/predictionengine/multi_head_self_attention.py(3 hunks)application/predictionengine/src/predictionengine/post_processor.py(2 hunks)application/predictionengine/tests/test_dataset.py(4 hunks)application/predictionengine/tests/test_gated_residual_network.py(5 hunks)application/predictionengine/tests/test_long_short_term_memory.py(5 hunks)application/predictionengine/tests/test_loss_function.py(3 hunks)application/predictionengine/tests/test_multi_head_self_attention.py(5 hunks)application/predictionengine/tests/test_post_processor.py(2 hunks)application/predictionengine/tests/test_ticker_embedding.py(1 hunks)infrastructure/__main__.py(1 hunks)infrastructure/services.py(1 hunks)workflows/prediction_model.py(4 hunks)
✅ Files skipped from review due to trivial changes (10)
- application/predictionengine/tests/test_ticker_embedding.py
- application/predictionengine/tests/test_post_processor.py
- application/predictionengine/tests/test_gated_residual_network.py
- application/predictionengine/src/predictionengine/loss_function.py
- application/predictionengine/src/predictionengine/models.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/tests/test_long_short_term_memory.py
- application/predictionengine/src/predictionengine/dataset.py
🚧 Files skipped from review as they are similar to previous changes (4)
- infrastructure/services.py
- infrastructure/main.py
- application/positionmanager/src/positionmanager/clients.py
- application/datamanager/src/datamanager/config.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
application/predictionengine/tests/test_loss_function.py (1)
application/predictionengine/src/predictionengine/loss_function.py (1)
quantile_loss(8-29)
application/positionmanager/src/positionmanager/main.py (4)
application/positionmanager/src/positionmanager/clients.py (3)
AlpacaClient(12-63)DataClient(66-103)get_data(70-103)application/datamanager/src/datamanager/models.py (1)
DateRange(31-46)application/positionmanager/src/positionmanager/models.py (4)
DateRange(39-61)Money(9-36)PredictionPayload(64-65)from_float(32-33)application/positionmanager/src/positionmanager/portfolio.py (1)
PortfolioOptimizer(9-62)
application/predictionengine/src/predictionengine/main.py (4)
application/predictionengine/src/predictionengine/dataset.py (1)
DataSet(18-214)application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (1)
load(168-173)application/predictionengine/src/predictionengine/models.py (1)
PredictionResponse(4-5)application/datamanager/src/datamanager/main.py (1)
lifespan(50-72)
🪛 Pylint (3.3.7)
application/predictionengine/src/predictionengine/post_processor.py
[refactor] 10-10: Too few public methods (1/2)
(R0903)
application/positionmanager/src/positionmanager/main.py
[refactor] 28-28: Too many local variables (16/15)
(R0914)
application/predictionengine/src/predictionengine/gated_residual_network.py
[refactor] 7-7: Too many instance attributes (8/7)
(R0902)
[refactor] 7-7: Too few public methods (1/2)
(R0903)
application/predictionengine/src/predictionengine/main.py
[error] 14-14: Attempted relative import beyond top-level package
(E0402)
[error] 15-15: Attempted relative import beyond top-level package
(E0402)
[error] 16-16: Attempted relative import beyond top-level package
(E0402)
[refactor] 99-99: Too many local variables (17/15)
(R0914)
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py
[error] 14-14: Attempted relative import beyond top-level package
(E0402)
[error] 15-15: Attempted relative import beyond top-level package
(E0402)
[error] 16-16: Attempted relative import beyond top-level package
(E0402)
[error] 17-17: Attempted relative import beyond top-level package
(E0402)
[error] 18-18: Attempted relative import beyond top-level package
(E0402)
[error] 19-19: Attempted relative import beyond top-level package
(E0402)
[error] 20-20: Attempted relative import beyond top-level package
(E0402)
[refactor] 24-24: Too many arguments (12/5)
(R0913)
[refactor] 24-24: Too many positional arguments (12/5)
(R0917)
🔇 Additional comments (28)
application/predictionengine/src/predictionengine/long_short_term_memory.py (3)
2-2: LGTM! Modern type hints and clean imports.Good modernization using lowercase built-in generics (
list,tuple) instead of theirtypingmodule counterparts, which is the preferred approach for Python 3.9+.Also applies to: 17-17, 25-25
24-26: LGTM! Proper handling of built-in name shadowing.Renaming the parameter from
inputtoinput_correctly avoids shadowing the built-ininputfunction, following Python naming conventions.Also applies to: 38-38
61-63: LGTM! Improved error handling clarity.Assigning the error message to a variable before raising the exception improves code readability and makes the error handling more explicit.
application/positionmanager/src/positionmanager/main.py (4)
2-14: LGTM! Import organization resolved.The import organization is now clean and follows Python conventions with proper grouping of standard library, third-party, and local imports. The duplicate import issues mentioned in previous reviews appear to have been successfully addressed.
28-28: LGTM! Modernized type hints.Good use of lowercase built-in generics (
dict) instead oftyping.Dict, which is the preferred approach for Python 3.9+.Also applies to: 137-137
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 28-28: Too many local variables (16/15)
(R0914)
45-49: LGTM! Improved exception handling specificity.Excellent improvement switching from generic
Exceptionto specific exceptions (requests.RequestException,APIError,ValidationError). This prevents catching unintended errors and the!rrepresentation in error messages provides better debugging information.Also applies to: 59-63, 72-76, 113-113, 147-148
52-53: LGTM! Timezone-aware datetime usage.Good practice making datetime objects timezone-aware using
datetime.now(tz=UTC). This prevents issues with naive datetime objects and is especially important in financial applications.application/predictionengine/src/predictionengine/gated_residual_network.py (3)
3-3: LGTM! Modern import organization and type hints.Good consolidation of imports and use of union type syntax (
int | None) instead ofOptional[int], which is preferred in Python 3.10+.Also applies to: 12-12
33-35: LGTM! Consistent built-in name shadowing avoidance.Parameter renaming from
inputtoinput_correctly avoids shadowing the built-in function and is consistent with similar changes across the prediction engine codebase.Also applies to: 42-44
46-46: LGTM! String literal in cast call.Using a string literal
"Tensor"in the cast call is good practice and can help avoid potential circular import issues.application/predictionengine/tests/test_loss_function.py (3)
3-3: LGTM! Improved random number generation consistency.Good practice using a dedicated random number generator (
PCG64) instead of the global numpy random state. This makes tests more reproducible and deterministic, which is especially important for numerical testing.Also applies to: 10-10, 58-59
21-21: LGTM! Flexible shape assertions.Good improvement making shape assertions more flexible by accepting multiple valid tensor shapes. This makes tests more robust when dealing with tensor operations that might return slightly different but equivalent shapes.
Also applies to: 32-32
29-29: LGTM! Explicit type hint for clarity.Adding the explicit type hint
loss: Tensorimproves code readability and makes the expected type clear in the test.workflows/prediction_model.py (3)
7-7: LGTM: Modern type annotations improve code clarity.The migration from
typing.List[Dict[str, Any]]tolist[dict[str, Any]]follows modern Python typing practices and improves readability.Also applies to: 14-14, 26-26
48-48: LGTM: Simplified workflow logic.Removing the unnecessary assignment and return statement simplifies the workflow function appropriately.
37-37:Details
❓ Verification inconclusive
Verify MODEL_BUCKET environment variable is always set.
The removal of the default fallback value could cause
bucket_pathto beNone, leading to potential runtime errors when creating thePathobject on line 39.
🏁 Script executed:
#!/bin/bash # Check if MODEL_BUCKET environment variable is set in infrastructure configuration rg -A 5 -B 5 "MODEL_BUCKET" --type toml --type py --type yamlLength of output: 684
Ensure MODEL_BUCKET Is Always Defined
Without a fallback,
bucket_path = os.getenv("MODEL_BUCKET")may becomeNone, leading to a runtime error when you build thePath. Please verify that every deployment and CI/CD configuration setsMODEL_BUCKET(for example, in Docker/Compose files, Kubernetes manifests, Terraform variables, GitHub Actions workflows,.envfiles, etc.). You might also consider:
- Switching to
os.environ["MODEL_BUCKET"]to fail fast if it’s unset- Adding an explicit check at startup:
bucket_path = os.getenv("MODEL_BUCKET") if not bucket_path: raise RuntimeError("Environment variable MODEL_BUCKET must be set")- Confirming in your infrastructure definitions that
MODEL_BUCKETis declared and populatedapplication/predictionengine/src/predictionengine/main.py (5)
18-18: LGTM: Constant eliminates magic numbers.Introducing
LOOKBACK_DAYSimproves maintainability by centralizing the 30-day lookback period used throughout the codebase.
21-23: LGTM: Custom exception improves error context.The
LoadErrorexception class provides better semantic meaning for model loading failures compared to generic exceptions.
26-26: LGTM: Modern type annotation for AsyncGenerator.Using
collections.abc.AsyncGeneratorinstead oftyping.AsyncGeneratorfollows current best practices.
103-103: LGTM: Timezone-aware datetime prevents ambiguity.Using
datetime.now(tz=UTC)ensures consistent behavior across different deployment environments and eliminates timezone-related bugs.
88-88: LGTM: Improved file handling and exception specificity.Using
Path.exists()instead ofos.path.exists()is more modern, and catching the specificLoadErrorprovides better error handling granularity.Also applies to: 92-93
application/predictionengine/src/predictionengine/post_processor.py (3)
7-7: LGTM: Type alias improves code readability.The
TensorMappingalias fordict[str, Tensor]eliminates repetition and makes the code more maintainable.
25-31: LGTM: Precise numpy typing enhances type safety.Using
npt.NDArray[np.float64]provides explicit dtype information, improving static analysis and documentation clarity.
47-48: LGTM: Clear error message assignment.Assigning the error message to a variable before raising improves code clarity without changing functionality.
application/predictionengine/src/predictionengine/miniature_temporal_fusion_transformer.py (4)
178-178: LGTM: Parameter renaming avoids builtin shadowing.Renaming
inputtoinput_prevents shadowing the built-ininput()function, which is a good practice for code clarity and avoiding potential conflicts.
33-34: LGTM: Modern type annotations improve clarity.The migration to built-in generic types and precise numpy typing (
npt.NDArray[np.float64]) enhances type safety and provides better static analysis support.Also applies to: 79-85
38-71: LGTM: Explicit attribute type annotations.Adding explicit type annotations for all attributes improves code documentation and enables better IDE support and static analysis.
136-136: LGTM: Proper handling of ignored return values.Assigning unused return values to
_makes the intent explicit and satisfies linting tools.Also applies to: 173-173
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
fix bandit issues fixing ruff issues
update to fix blind exceptions
add bugbear add comma linting fix bugbear and commas add timezones
no fixme/todos implicit string concatenation import conventions logging pathlib
sort imports fix naming conventions
fix bandit issues fixing ruff issues
update to fix blind exceptions
add bugbear add comma linting fix bugbear and commas add timezones
no fixme/todos implicit string concatenation import conventions logging pathlib
sort imports fix naming conventions
There was a problem hiding this comment.
@chrisaddy approved but tests/checks are failing.
2 participants
This pull request introduces significant updates across multiple areas, including dependency management, infrastructure refactoring, service creation, and application configuration. The changes aim to enhance maintainability, modularity, and functionality of the codebase. Below is a summary of the most important changes grouped by theme.
Dependency Management
Added new dependencies to support metrics instrumentation and logging in
positionmanagerandpredictionengineservices, includingprometheus-fastapi-instrumentatorandloguru. (application/positionmanager/pyproject.toml,application/predictionengine/pyproject.toml) [1] [2]Introduced a
devdependency group indatamanagerfor development tools, such asbehave. (application/datamanager/pyproject.toml)Infrastructure Refactoring
Consolidated service creation logic into a reusable
create_servicefunction, simplifying the definition of Cloud Run services. (infrastructure/services.py)Replaced the previous
cloud_run.pyfile with a modular and DRY approach, moving environment variable handling toenvironment_variables.pyand service creation toservices.py. (infrastructure/__main__.py,infrastructure/environment_variables.py) [1] [2]Updated the
infrastructureproject version to reflect the new release cycle. (infrastructure/pyproject.toml,pyproject.toml) [1] [2]Application Configuration
Added a
compose.yamlfile for thepredictionengineservice to enable local integration testing with Docker Compose. (application/predictionengine/compose.yaml)Updated the
lifespanfunction signatures indatamanagerandpredictionengineto include the type hint for theAsyncGeneratorreturn value. (application/datamanager/src/datamanager/main.py,application/predictionengine/src/predictionengine/main.py) [1] [2]These changes collectively improve the maintainability, scalability, and observability of the system while ensuring consistency across services.
Summary by CodeRabbit
New Features
Dependency Updates
Infrastructure
Versioning
Other