Consolidate package versions/convert Python to virtual workspace#766
Consolidate package versions/convert Python to virtual workspace#766forstmeier merged 10 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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 ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMultiple manifest files had their project/package versions changed from 0.1.0 to 0.0.1; several Python projects added or adjusted dependencies and tooling config; combine_data.py was updated to use equity_details_schema and renamed parameters/variables; a minor lint suppression was added; root pyproject.toml project metadata and a sources mapping were removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryConsolidated all package versions to
The workspace structure is now properly configured with all dependencies correctly declared in their respective packages. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| pyproject.toml | Removed project-level metadata, now configured as Python virtual workspace with dev dependencies |
| applications/equitypricemodel/pyproject.toml | Updated version to 0.0.1, adjusted internal dependency constraint, added runtime dependencies (fastapi, polars, pandera, requests) |
| tools/pyproject.toml | Updated version to 0.0.1, added missing dependencies (sagemaker, structlog, requests, polars) previously addressed in feedback |
| applications/equitypricemodel/src/equitypricemodel/combine_data.py | Renamed variables and schema from categories to equity_details, updated log messages per previous feedback |
Last reviewed commit: 8a51dd1
There was a problem hiding this comment.
Pull request overview
This pull request consolidates package versions across the codebase to use SemVer format 0.0.1 and converts the Python project structure to a virtual workspace. The changes involve version standardization across both Python and Rust packages, removal of the root project definition in favor of a workspace-only configuration, and an incomplete refactoring of "categories" to "equity_details" terminology in the equitypricemodel application.
Changes:
- Standardized all package versions to
0.0.1across Python and Rust projects - Converted root
pyproject.tomlto workspace-only configuration by removing the[project]section - Updated internal dependency references from
0.1.0to0.0.1 - Added
noqa: PERF203comment to suppress performance warning for intentional exception handling in loop - Partially refactored equity categories schema and variable naming to use "equity_details" terminology
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Removed root project definition, keeping only workspace configuration |
| tools/pyproject.toml | Updated version from 0.1.0 to 0.0.1 |
| libraries/python/pyproject.toml | Updated version from 0.1.0 to 0.0.1 |
| infrastructure/pyproject.toml | Updated version from 0.1.0 to 0.0.1 |
| applications/portfoliomanager/pyproject.toml | Updated version and internal dependency to 0.0.1 |
| applications/equitypricemodel/pyproject.toml | Updated version and internal dependency to 0.0.1 |
| applications/datamanager/Cargo.toml | Updated version from 0.1.0 to 0.0.1 |
| applications/portfoliomanager/src/portfoliomanager/server.py | Added noqa comment to suppress PERF203 warning |
| applications/equitypricemodel/src/equitypricemodel/combine_data.py | Renamed schema import and updated variable references (incomplete) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
applications/portfoliomanager/pyproject.toml (1)
6-12:⚠️ Potential issue | 🟠 MajorAdd
structlogas a direct dependency.
structlogis imported and used throughout the package (imported inserver.py,risk_management.py, andalpaca_client.py), but it is not declared independencies. It must be added explicitly to ensure reliable dependency resolution across environments.Proposed fix
dependencies = [ "internal>=0.0.1", "httpx>=0.27.0", "pandera[polars,pandas]>=0.26.0", "alpaca-py>=0.42.1", "sentry-sdk[fastapi]>=2.0.0", + "structlog>=25.5.0", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/portfoliomanager/pyproject.toml` around lines 6 - 12, Add structlog as a direct dependency in the project's dependency list: update the dependencies array in pyproject.toml to include "structlog>=21.5.0" (or a minimal compatible version you choose) alongside the existing entries; this ensures imports of structlog in server.py, risk_management.py, and alpaca_client.py resolve reliably across environments. Make sure to follow the same quoting/format style used for other entries and run dependency tooling (e.g., poetry/pip) to lock/install the new requirement.applications/equitypricemodel/pyproject.toml (1)
3-14:⚠️ Potential issue | 🟠 MajorMove
structlogto root workspacepyproject.toml— it's used but undeclared inportfoliomanagerandtools.
structlogis imported in three workspace members (equitypricemodel, portfoliomanager, tools) but is only explicitly declared inequitypricemodel/pyproject.toml.portfoliomanagerandtoolscurrently rely on this transitive dependency, creating an implicit dependency chain. Instead of duplicating the declaration across members, addstructlog>=25.5.0to the root workspacepyproject.tomldependencies so all members inherit it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/equitypricemodel/pyproject.toml` around lines 3 - 14, Add structlog>=25.5.0 to the root workspace pyproject.toml top-level [project] or dependencies list so all members (equitypricemodel, portfoliomanager, tools) declare it transitively; update the root pyproject.toml dependencies to include "structlog>=25.5.0" and remove the duplicate "structlog>=25.5.0" entry from equitypricemodel/pyproject.toml to avoid duplicated declarations while ensuring portfoliomanager and tools no longer rely on an implicit transitive dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/equitypricemodel/src/equitypricemodel/combine_data.py`:
- Around line 26-28: Update the exception log message to reflect the renamed
schema: replace the old "Categories data validation failed" text with something
like "Equity details data validation failed" in the logger.exception call inside
the exception handler that catches Exception as e (the block referencing
equity_details_schema validation). Keep passing the error details (error=str(e))
and re-raise as currently implemented.
---
Outside diff comments:
In `@applications/equitypricemodel/pyproject.toml`:
- Around line 3-14: Add structlog>=25.5.0 to the root workspace pyproject.toml
top-level [project] or dependencies list so all members (equitypricemodel,
portfoliomanager, tools) declare it transitively; update the root pyproject.toml
dependencies to include "structlog>=25.5.0" and remove the duplicate
"structlog>=25.5.0" entry from equitypricemodel/pyproject.toml to avoid
duplicated declarations while ensuring portfoliomanager and tools no longer rely
on an implicit transitive dependency.
In `@applications/portfoliomanager/pyproject.toml`:
- Around line 6-12: Add structlog as a direct dependency in the project's
dependency list: update the dependencies array in pyproject.toml to include
"structlog>=21.5.0" (or a minimal compatible version you choose) alongside the
existing entries; this ensures imports of structlog in server.py,
risk_management.py, and alpaca_client.py resolve reliably across environments.
Make sure to follow the same quoting/format style used for other entries and run
dependency tooling (e.g., poetry/pip) to lock/install the new requirement.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
applications/datamanager/Cargo.tomlapplications/equitypricemodel/pyproject.tomlapplications/equitypricemodel/src/equitypricemodel/combine_data.pyapplications/portfoliomanager/pyproject.tomlapplications/portfoliomanager/src/portfoliomanager/server.pyinfrastructure/pyproject.tomllibraries/python/pyproject.tomlpyproject.tomltools/pyproject.toml
💤 Files with no reviewable changes (1)
- pyproject.toml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/equitypricemodel/src/equitypricemodel/combine_data.py (1)
85-87:⚠️ Potential issue | 🟡 MinorStale CLI error message still references "categories CSV".
The
__main__usage error on line 86 still says"Requires categories CSV, equity bars CSV, and output CSV paths as arguments". The first argument is now the equity details CSV path.📝 Proposed fix
- "Requires categories CSV, equity bars CSV, and output CSV paths as arguments", # noqa: E501 + "Requires equity details CSV, equity bars CSV, and output CSV paths as arguments", # noqa: E501🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/equitypricemodel/src/equitypricemodel/combine_data.py` around lines 85 - 87, Update the stale CLI error text in combine_data.py: replace the logger.error message that currently says "Requires categories CSV, equity bars CSV, and output CSV paths as arguments" with the correct argument order and wording (e.g., "Requires equity details CSV, equity bars CSV, and output CSV paths as arguments") in the module's __main__/argument-check block so the usage error refers to the equity details CSV as the first argument (locate the logger.error call in combine_data.py near the CLI/if __name__ == '__main__' section).
♻️ Duplicate comments (1)
applications/equitypricemodel/src/equitypricemodel/combine_data.py (1)
28-30:⚠️ Potential issue | 🟡 MinorStale log message — "Categories data validation failed" was not updated.
Line 29 was flagged in the previous review and the developer marked it as fixed, but the message is unchanged in the current code. It should read something like
"Equity details data validation failed".📝 Proposed fix
- logger.exception("Categories data validation failed", error=str(e)) + logger.exception("Equity details data validation failed", error=str(e))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/equitypricemodel/src/equitypricemodel/combine_data.py` around lines 28 - 30, The log message in the exception handler inside combine_data.py is stale; update the logger.exception call in the except Exception as e block so the message reads "Equity details data validation failed" (keep reporting the exception details as before via the logger.exception call on the same handler). Locate the except block that calls logger.exception("Categories data validation failed", error=str(e)) and replace the message text while ensuring the exception details continue to be included (e.g., using logger.exception with the new message and current exception info).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@applications/equitypricemodel/src/equitypricemodel/combine_data.py`:
- Around line 85-87: Update the stale CLI error text in combine_data.py: replace
the logger.error message that currently says "Requires categories CSV, equity
bars CSV, and output CSV paths as arguments" with the correct argument order and
wording (e.g., "Requires equity details CSV, equity bars CSV, and output CSV
paths as arguments") in the module's __main__/argument-check block so the usage
error refers to the equity details CSV as the first argument (locate the
logger.error call in combine_data.py near the CLI/if __name__ == '__main__'
section).
---
Duplicate comments:
In `@applications/equitypricemodel/src/equitypricemodel/combine_data.py`:
- Around line 28-30: The log message in the exception handler inside
combine_data.py is stale; update the logger.exception call in the except
Exception as e block so the message reads "Equity details data validation
failed" (keep reporting the exception details as before via the logger.exception
call on the same handler). Locate the except block that calls
logger.exception("Categories data validation failed", error=str(e)) and replace
the message text while ensuring the exception details continue to be included
(e.g., using logger.exception with the new message and current exception info).
Additional Comments (2)
With the
These were previously provided by the root-level Context Used: Context from Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/portfoliomanager/pyproject.toml
Line: 6-12
Comment:
**Missing dependencies after root project removal**
With the `[project]` section removed from the root `pyproject.toml`, several dependencies that `portfoliomanager` uses at runtime are no longer declared anywhere in its dependency chain:
- **`structlog`**: imported in `server.py`, `risk_management.py`, and `alpaca_client.py`
- **`requests`**: imported in `server.py` (used for `requests.get()` and `requests.post()` calls to datamanager)
- **`uvicorn`**: invoked in the `Dockerfile` ENTRYPOINT
- **`fastapi`**: imported in `server.py` — currently only available transitively via `sentry-sdk[fastapi]`, which is fragile
These were previously provided by the root-level `[project]` dependencies. Without them declared here, `uv run --package portfoliomanager` will fail to resolve these imports.
```suggestion
"internal>=0.0.1",
"fastapi>=0.121.0",
"uvicorn>=0.35.0",
"structlog>=25.5.0",
"requests>=2.32.5",
"httpx>=0.27.0",
"pandera[polars,pandas]>=0.26.0",
"alpaca-py>=0.42.1",
"sentry-sdk[fastapi]>=2.0.0",
```
**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=ce2b8f3e-a42c-4f67-b7b7-b8b03e89616e))
How can I resolve this? If you propose a fix, please make it concise.
After removing the root-level
Additionally, Context Used: Context from Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/equitypricemodel/pyproject.toml
Line: 6-14
Comment:
**Missing `uvicorn` and `fastapi` dependencies**
After removing the root-level `[project]` section, this package no longer inherits `uvicorn` and `fastapi`. Both are required:
- **`uvicorn`**: invoked in the `Dockerfile` ENTRYPOINT (`uvicorn equitypricemodel.server:application ...`)
- **`fastapi`**: imported in `server.py` — currently only available transitively via `sentry-sdk[fastapi]`, which is fragile
Additionally, `mypy-boto3-s3` is imported under `TYPE_CHECKING` in `server.py` and is no longer declared anywhere.
```suggestion
"internal>=0.0.1",
"boto3>=1.35.0",
"python-dotenv>=1.2.1",
"fastapi>=0.121.0",
"uvicorn>=0.35.0",
"sentry-sdk[fastapi]>=2.0.0",
"structlog>=25.5.0",
"tinygrad>=0.10.3",
"numpy>=1.26.4",
"mypy-boto3-s3>=1.42.37",
```
**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=ce2b8f3e-a42c-4f67-b7b7-b8b03e89616e))
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/equitypricemodel/pyproject.toml
Line: 6-14
Comment:
Missing `fastapi` and `requests` dependencies
`server.py` imports both `fastapi` and `requests`, which were previously provided by the root-level `[project]` dependencies. Add them here:
```suggestion
dependencies = [
"internal>=0.0.1",
"boto3>=1.35.0",
"python-dotenv>=1.2.1",
"sentry-sdk[fastapi]>=2.0.0",
"structlog>=25.5.0",
"tinygrad>=0.10.3",
"numpy>=1.26.4",
"fastapi>=0.121.0",
"requests>=2.32.5",
]
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/portfoliomanager/pyproject.toml
Line: 6-12
Comment:
Missing `structlog`, `requests`, and `uvicorn` dependencies
`server.py`, `alpaca_client.py`, and `risk_management.py` import `structlog`. `server.py` also imports `requests`. Additionally, `uvicorn` is needed to run the FastAPI server. These were previously provided by the root-level `[project]` dependencies:
```suggestion
dependencies = [
"internal>=0.0.1",
"httpx>=0.27.0",
"pandera[polars,pandas]>=0.26.0",
"alpaca-py>=0.42.1",
"sentry-sdk[fastapi]>=2.0.0",
"structlog>=25.5.0",
"requests>=2.32.5",
"uvicorn>=0.35.0",
]
```
How can I resolve this? If you propose a fix, please make it concise. |
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/portfoliomanager/pyproject.toml
Line: 6-12
Comment:
Missing `requests` dependency that was previously provided by root workspace.
`server.py` imports and uses `requests` for HTTP calls to datamanager (lines 368, 425) and exception handling (line 404).
```suggestion
dependencies = [
"internal>=0.0.1",
"httpx>=0.27.0",
"pandera[polars,pandas]>=0.26.0",
"alpaca-py>=0.42.1",
"sentry-sdk[fastapi]>=2.0.0",
"requests>=2.32.5",
]
```
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/equitypricemodel/pyproject.toml
Line: 6-14
Comment:
Missing `requests`, `fastapi`, and `uvicorn` dependencies. `server.py` imports these but they're not declared.
`requests` is imported on line 15 of `server.py` but isn't available as a transitive dependency (unlike `portfoliomanager`, which gets it from `alpaca-py`).
`fastapi` is pulled in via `sentry-sdk[fastapi]`, but relying on this is fragile - if the sentry-sdk extra changes, imports will break.
`uvicorn` isn't declared anywhere and will be missing when running the server.
```suggestion
dependencies = [
"internal>=0.0.1",
"boto3>=1.35.0",
"python-dotenv>=1.2.1",
"sentry-sdk[fastapi]>=2.0.0",
"structlog>=25.5.0",
"tinygrad>=0.10.3",
"numpy>=1.26.4",
"requests>=2.32.5",
"fastapi>=0.121.0",
"uvicorn>=0.35.0",
]
```
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: applications/portfoliomanager/pyproject.toml
Line: 6-12
Comment:
Missing explicit `fastapi`, `uvicorn`, `structlog`, and `requests` declarations. While `fastapi` comes from `sentry-sdk[fastapi]` and `requests` from `alpaca-py`, directly importing them without declaring them is fragile.
```suggestion
dependencies = [
"internal>=0.0.1",
"httpx>=0.27.0",
"pandera[polars,pandas]>=0.26.0",
"alpaca-py>=0.42.1",
"sentry-sdk[fastapi]>=2.0.0",
"fastapi>=0.121.0",
"uvicorn>=0.35.0",
"structlog>=25.5.0",
"requests>=2.32.5",
]
```
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
applications/portfoliomanager/pyproject.toml (1)
7-15: Duplicate dependency centralization already noted.
Same workspace-level duplication concern applies here; see earlier note on moving shared deps to the root workspacepyproject.toml.#!/bin/bash # Verify duplicate dependency declarations across workspace pyproject.toml files. rg -n --glob 'pyproject.toml' -e 'requests>=|polars>=|structlog>=|fastapi>='Based on learnings: Move duplicate dependencies into root workspace
pyproject.toml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/portfoliomanager/pyproject.toml` around lines 7 - 15, This pyproject lists dependencies duplicated elsewhere; remove the shared packages (fastapi, requests, polars, structlog and any other workspace-common libs like pandera/alpaca-py/sentry-sdk if present at root) from this application's pyproject.toml and add them to the root workspace pyproject.toml dependency section instead, ensuring version specifiers are preserved and this module only keeps app-specific deps; update the workspace root pyproject.toml to include the consolidated entries (matching versions) and then delete the duplicate lines from the application's dependency list to avoid conflicts.applications/equitypricemodel/pyproject.toml (1)
7-20: Duplicate dependency centralization already noted.
Same workspace-level duplication concern applies here; see earlier note on moving shared deps to the root workspacepyproject.toml.#!/bin/bash # Verify duplicate dependency declarations across workspace pyproject.toml files. rg -n --glob 'pyproject.toml' -e 'requests>=|polars>=|structlog>=|fastapi>=|boto3>='Based on learnings: Move duplicate dependencies into root workspace
pyproject.toml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/equitypricemodel/pyproject.toml` around lines 7 - 20, This pyproject lists dependencies already declared at the workspace root; remove duplicate entries (e.g. requests, polars, structlog, fastapi, boto3, numpy, pandera, sentry-sdk, tinygrad, internal) from this package's dependencies and instead reference them from the root workspace pyproject.toml, and if package-specific extras or pinned versions are needed reconcile them at the root; also ensure the dev dependency group (boto3-stubs[s3]) is moved or referenced consistently at the workspace level to avoid conflicting declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/pyproject.toml`:
- Around line 6-15: Remove duplicate pins for "polars>=1.29.0",
"requests>=2.32.5", and "structlog>=25.5.0" from the tools pyproject.toml
dependencies array and instead declare them once in the root workspace
pyproject.toml; update the tools/tools/pyproject.toml to only include packages
unique to tools (e.g., keep "boto3>=1.40.74" and "sagemaker>=2.256.0,<3.0.0")
and remove the repeated entries so version management for these libraries is
centralized in the root workspace pyproject.toml.
---
Duplicate comments:
In `@applications/equitypricemodel/pyproject.toml`:
- Around line 7-20: This pyproject lists dependencies already declared at the
workspace root; remove duplicate entries (e.g. requests, polars, structlog,
fastapi, boto3, numpy, pandera, sentry-sdk, tinygrad, internal) from this
package's dependencies and instead reference them from the root workspace
pyproject.toml, and if package-specific extras or pinned versions are needed
reconcile them at the root; also ensure the dev dependency group
(boto3-stubs[s3]) is moved or referenced consistently at the workspace level to
avoid conflicting declarations.
In `@applications/portfoliomanager/pyproject.toml`:
- Around line 7-15: This pyproject lists dependencies duplicated elsewhere;
remove the shared packages (fastapi, requests, polars, structlog and any other
workspace-common libs like pandera/alpaca-py/sentry-sdk if present at root) from
this application's pyproject.toml and add them to the root workspace
pyproject.toml dependency section instead, ensuring version specifiers are
preserved and this module only keeps app-specific deps; update the workspace
root pyproject.toml to include the consolidated entries (matching versions) and
then delete the duplicate lines from the application's dependency list to avoid
conflicts.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
applications/equitypricemodel/pyproject.tomlapplications/portfoliomanager/pyproject.tomlpyproject.tomltools/pyproject.toml
💤 Files with no reviewable changes (1)
- pyproject.toml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Overview
Changes
0.0.1Context
Just another little tweak before getting into the bulk of the bigger projects.
Summary by CodeRabbit
Chores
New Dependencies
Bug Fixes / Maintenance