Docker service image fixes and updates

[forstmeier]

Overview

Changes

• manually upload details.csv to S3
• update datamanager "equity details" endpoints for direct load/"not implemented"
• add various fixes and expand tests

Context

More bugs I ran into backfilling the data.

Summary by CodeRabbit

• New Features

  • Sync requests for weekend dates now return success and skip processing.

• Bug Fixes

  • API error logging avoids exposing sensitive request details.

• Changes

  • Equity details endpoint now returns raw CSV content; synchronous update path marked not implemented.
  • S3 equity-details key/path updated and related tooling/tests adjusted.
  • Simplified runtime library path handling and updated backend service URL.
  • Added uvicorn to Python service dependencies.

• Tests

  • Test expectations and fixtures updated to match endpoint and storage path changes.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e45832b and 287abe8.

📒 Files selected for processing (2)

• tools/src/tools/prepare_training_data.py
• tools/tests/test_prepare_training_data.py

---

📝 Walkthrough

Walkthrough

Replaced dynamic ENV path concatenation in datamanager Dockerfile with fixed paths; equity-bars now short-circuits weekend dates; equity-details now reads and returns raw CSV from S3 and its sync handler is NOT_IMPLEMENTED; renamed S3 key and added CSV-read helper; updated tests, two pyproject deps, and MASSIVE_BASE_URL.

Changes

Cohort / File(s)	Summary
Dockerfile applications/datamanager/Dockerfile	Replaced conditional concatenation for LIBRARY_PATH, LD_LIBRARY_PATH, and PKG_CONFIG_PATH with fixed /usr/local/lib and /usr/local/lib/pkgconfig in both ENV blocks.
Equity Bars Handler applications/datamanager/src/equity_bars.rs	Added weekend check to short-circuit POST /equity-bars (returns 200 with message for Saturday/Sunday); added Datelike/Weekday imports and masked API URL in error logging.
Equity Details Handler applications/datamanager/src/equity_details.rs	Switched GET /equity-details to read raw CSV via read_equity_details_csv_from_s3 and return text/csv; removed in-process sync/dataframe CSV creation and made sync endpoint return NOT_IMPLEMENTED; adjusted logging levels.
Storage Layer applications/datamanager/src/storage.rs	Renamed S3 key constant to EQUITY_DETAILS_KEY; added pub async fn read_equity_details_csv_from_s3(state: &State) -> Result<String, Error> and updated dataframe helper to parse CSV from that function.
Tests applications/datamanager/tests/test_handlers.rs, applications/datamanager/tests/test_storage.rs	Updated S3 key expectations from equity/details/categories.csv → equity/details/details.csv; repurposed/removed many equity-details tests in favor of equity-bars semantics including weekend behavior; adjusted storage tests accordingly.
Tools tools/src/tools/prepare_training_data.py, tools/tests/test_prepare_training_data.py	Updated S3 key used to read categories to equity/details/details.csv and corresponding test expectation.
Python Projects applications/equitypricemodel/pyproject.toml, applications/portfoliomanager/pyproject.toml	Added dependency uvicorn>=0.34.0 to both projects.
Infrastructure infrastructure/__main__.py	Changed datamanager container MASSIVE_BASE_URL from https://api.massive.io to https://api.massive.com.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Client as Client
participant Server as Datamanager
participant Massive as Massive API
participant S3 as S3
Client->>Server: POST /equity-bars { date }
alt date is weekend
Server-->>Client: 200 OK (skip processing, message)
else weekday
Server->>Massive: fetch bars
Massive-->>Server: bars data / error
Server->>S3: optional read/write equity details
S3-->>Server: CSV content / error
Server-->>Client: 200 OK / error

mermaid
sequenceDiagram
participant Client as Client
participant Server as Datamanager
participant S3 as S3
Client->>Server: GET /equity-details
Server->>S3: read_equity_details_csv_from_s3()
alt S3 returns CSV
S3-->>Server: CSV (utf-8)
Server-->>Client: 200 OK (text/csv)
else S3 error
S3-->>Server: Error
Server-->>Client: 500 Internal Server Error

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Refactor datamanager module #635 — Overlapping edits to datamanager internals (equity_bars.rs, storage.rs).
• Model rebuild #637 — Strong overlap: equity_details/equity_bars/storage changes match edits in this PR.
• infra cleanups #754 — Related infra change touching datamanager container environment / base URL.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The PR title 'Docker service image fixes and updates' is vague and generic, using non-descriptive terms that fail to convey the actual changes, which primarily involve endpoint refactoring, S3 key renaming, and test updates across multiple services.	Use a more specific title that reflects the main changes, such as 'Refactor equity details endpoint and update S3 data paths' or 'Update datamanager endpoints and consolidate S3 equity details retrieval'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 81.25% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch docker-service-image-fixes-and-updates

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Updates service dependencies and datamanager behavior to support backfill-related fixes, including shifting equity details to a direct S3 CSV read path and adding guardrails around equity bars syncing.

Changes:

• Add uvicorn to Python service dependencies (and uv.lock) for FastAPI runtimes.
• Update datamanager equity details: use equity/details/details.csv and return 501 Not Implemented for sync; adjust handlers/storage and tests accordingly.
• Add weekend short-circuit for /equity-bars sync; update Dockerfile env var handling and align Cargo lock versioning.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
uv.lock	Adds uvicorn to the workspace lock to match service runtime needs.
infrastructure/main.py	Updates the MASSIVE_BASE_URL value passed into the datamanager task.
applications/portfoliomanager/pyproject.toml	Adds uvicorn dependency for serving FastAPI.
applications/equitypricemodel/pyproject.toml	Adds uvicorn dependency for serving FastAPI.
applications/datamanager/tests/test_storage.rs	Updates tests to use the new equity details S3 key.
applications/datamanager/tests/test_handlers.rs	Reworks tests for equity details sync (now not implemented) and adds weekend NO_CONTENT coverage for equity bars.
applications/datamanager/src/storage.rs	Renames equity details key and adds an API to read raw CSV content from S3.
applications/datamanager/src/equity_details.rs	Switches GET to return raw CSV from S3; sync now returns NOT_IMPLEMENTED.
applications/datamanager/src/equity_bars.rs	Skips weekend dates and tweaks error logging.
applications/datamanager/Dockerfile	Attempts to improve env var concatenation for library paths.
Cargo.lock	Aligns datamanager package version with its Cargo.toml.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[greptile-apps]

Greptile Summary

This PR fixes missing dependencies and simplifies the equity details workflow by switching from automated sync to manual CSV uploads.

• Fixed critical bug where uvicorn dependency was missing from equitypricemodel and portfoliomanager pyproject.toml files, even though it's required by their Dockerfile entrypoints
• Added weekend date validation in equity bars sync to skip API calls on Saturdays/Sundays when markets are closed
• Improved security by using .without_url() in error logging to prevent exposing sensitive URL information
• Simplified equity details endpoint to return raw CSV content instead of DataFrame conversion
• Changed equity details sync endpoint to return NOT_IMPLEMENTED, shifting to manual CSV uploads to S3
• Renamed S3 key from equity/details/categories.csv to equity/details/details.csv consistently across all references (Rust code, Python tools, and tests)
• Updated Massive API base URL from https://api.massive.io to https://api.massive.com
• Simplified Dockerfile environment variable assignments (minor redundancy on lines 24-26 and 54-55, but harmless)

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• All changes are well-tested with updated test suites, address real bugs (missing uvicorn dependencies), improve security (error logging without URLs), and simplify the equity details workflow as intended. The changes are consistent across the codebase with proper S3 key updates in all references.
• No files require special attention

Important Files Changed

Filename	Overview
applications/datamanager/src/equity_bars.rs	Added weekend date checking to skip API calls on Saturdays/Sundays, and improved error logging security by using .without_url() to prevent exposing sensitive URL information
applications/datamanager/src/equity_details.rs	Removed complex sync logic (200+ lines) in favor of manual CSV uploads to S3, simplified get endpoint to return raw CSV content instead of converting from DataFrame
applications/datamanager/src/storage.rs	Changed S3 key from categories.csv to details.csv, refactored to add read_equity_details_csv_from_s3 function that returns raw CSV string
applications/datamanager/Dockerfile	Simplified environment variable assignments by removing conditional expansion syntax; variables now set twice (lines 24-26 and 54-55) which is redundant but harmless
infrastructure/main.py	Changed Massive API base URL from https://api.massive.io to https://api.massive.com
applications/equitypricemodel/pyproject.toml	Added missing uvicorn>=0.34.0 dependency which is required by the Dockerfile entrypoint
applications/portfoliomanager/pyproject.toml	Added missing uvicorn>=0.34.0 dependency which is required by the Dockerfile entrypoint

_{Last reviewed commit: 287abe8}

[coderabbitai]

Actionable comments posted: 1

🤖 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/datamanager/src/storage.rs`:
- Around line 757-787: The S3 object retrieval, body collection and UTF-8
conversion are duplicated between read_equity_details_csv_from_s3 and
read_equity_details_dataframe_from_s3; refactor by extracting the shared logic
into a single helper (e.g., read_s3_object_as_string or similar) that accepts
State and key (or have read_equity_details_dataframe_from_s3 simply call
read_equity_details_csv_from_s3) and returns Result<String, Error>, reuse the
same error mapping and logging, and update both read_equity_details_csv_from_s3
and read_equity_details_dataframe_from_s3 to call that helper to avoid
duplication.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e257323 and 540c174.

⛔ Files ignored due to path filters (2)

• Cargo.lock is excluded by !**/*.lock
• uv.lock is excluded by !**/*.lock

📒 Files selected for processing (9)

• applications/datamanager/Dockerfile
• applications/datamanager/src/equity_bars.rs
• applications/datamanager/src/equity_details.rs
• applications/datamanager/src/storage.rs
• applications/datamanager/tests/test_handlers.rs
• applications/datamanager/tests/test_storage.rs
• applications/equitypricemodel/pyproject.toml
• applications/portfoliomanager/pyproject.toml
• infrastructure/__main__.py

[greptile-apps]

_{11 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

applications/datamanager/src/equity_details.rs
client-facing error message includes internal error details which could expose system information

            warn!("Failed to fetch equity details from S3: {}", err);
            (
                StatusCode::INTERNAL_SERVER_ERROR,
                "Failed to fetch equity details",
            )

Prompt To Fix With AI

This is a comment left during a code review.
Path: applications/datamanager/src/equity_details.rs
Line: 24-28

Comment:
client-facing error message includes internal error details which could expose system information

```suggestion
            warn!("Failed to fetch equity details from S3: {}", err);
            (
                StatusCode::INTERNAL_SERVER_ERROR,
                "Failed to fetch equity details",
            )
```

How can I resolve this? If you propose a fix, please make it concise.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

applications/datamanager/src/equity_details.rs:28

• The HTTP 500 response body includes the underlying internal error string (format!("Failed to fetch equity details: {}", err)). Since this error can include AWS/S3 details (bucket/key/request IDs) and other internal context, it can unintentionally leak operational information to clients. Prefer returning a generic client-facing message (and keep the detailed error in logs only).

            warn!("Failed to fetch equity details from S3: {}", err);
            (
                StatusCode::INTERNAL_SERVER_ERROR,
                format!("Failed to fetch equity details: {}", err),
            )

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.