Conversation
|
Warning Rate limit exceeded@chrisaddy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 28 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 (7)
""" WalkthroughThe changes introduce a Docker-based testing workflow, update development and test dependencies, and streamline configuration files. Python version requirements are adjusted, coverage and pytest settings are enhanced, and service Dockerfiles are refactored for clarity and consistency. The GitHub Actions workflow is simplified to run tests only on Ubuntu, while Docker Compose now supports a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Docker Compose
participant Test Container
participant Coverage Tools
Developer->>Docker Compose: Run `docker compose run tests`
Docker Compose->>Test Container: Build & start with Dockerfile.test
Test Container->>Test Container: Install dependencies (uv sync)
Test Container->>Test Container: Set up pytest and coverage env vars
Test Container->>Test Container: Run pytest with coverage
Test Container->>Coverage Tools: Combine and report coverage
Test Container->>Developer: Output coverage reports
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
chrisaddy
force-pushed
the
nix-ci
branch
9 times, most recently
from
a70a97e to
5532068
Compare
chrisaddy
force-pushed
the
nix-ci
branch
2 times, most recently
from
1ca1399 to
5263466
Compare
set up nix for full ci/cd goodness set up nix gha action for ci removing nix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.github/workflows/test.yaml (1)
29-29:⚠️ Potential issueMismatch in coverage report filename
Thetestsservice generatescoverage.xmlby default, butpath-to-lcovis set to.python_coverage.xml. Coveralls won’t find the report as-is.Please update
path-to-lcovtocoverage.xmlor configure the test container to emit.python_coverage.xml.
🧹 Nitpick comments (8)
pyproject.toml (1)
29-43: Review runtime vs. dev dependency placement.
coverageandpytestappear in both the maindependenciesarray and thedevgroup. Consider removing testing tools from runtime dependencies to reduce production install size and potential attack surface.Dockerfile.test (1)
8-9: Syncing all dependency groups for tests.
Copying just thepyproject.tomland runninguv sync --all-groupsensures dev dependencies are available. Consider also copying a lock file for fully reproducible builds.application/positionmanager/Dockerfile (1)
1-1: Optional: usepython:3.13-slimfor smaller images.
Switching to the fullpython:3.13image works, but if no OS-level packages are required, usingpython:3.13-slimreduces image size and attack surface..mise.toml (2)
3-3: Suggest explicitly specifying dev dependency groups
Theuv synccommand now relies on default settings; consider adding--all-groupsfor clarity and to ensure dev dependencies (e.g., linters, test frameworks) are installed when running tasks locally.
30-30: Clean up test container after execution
When runningdocker compose run tests, adding the--rmflag will automatically remove stopped containers, preventing resource buildup over repeated runs.compose.yaml (1)
9-16: Consider refactoring complex command
The multiline shell command undercommandcould be moved into an entrypoint script in the image for readability, easier maintenance, and better layer caching.application/datamanager/Dockerfile (1)
21-23: Remove commented-out legacy entrypoint
Consider deleting the commented-outENTRYPOINTline to keep the Dockerfile clean, or relocate it into documentation if needed for historical reference..github/workflows/test.yaml (1)
7-13: Clean up commented-out matrix configuration
Keeping commented strategy lines can clutter the workflow. Remove them if cross-OS testing is no longer required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/test.yaml(1 hunks).gitignore(1 hunks).mise.toml(2 hunks)Dockerfile.test(1 hunks)application/datamanager/Dockerfile(1 hunks)application/datamanager/pyproject.toml(1 hunks)application/positionmanager/Dockerfile(1 hunks)compose.yaml(1 hunks)pyproject.toml(2 hunks)
🔇 Additional comments (20)
.gitignore (1)
9-12: Coverage files ignored to avoid VCS clutter.
The new ignore patterns.coverage*,.coverage/,coverage.xml, and.python_coverage.xmlcorrectly prevent coverage artifacts from being checked in as part of the Dockerized test workflows.pyproject.toml (3)
54-57: Coverage run configuration looks good.
Omitting__init__.pyand test files with parallel mode is appropriate for accurate metrics.
58-61: Coverage reporting settings are sound.
show_missingandskip_coveredwill highlight gaps without overwhelming reports.
62-63: XML output target matches ignore rules.
Specifying.python_coverage.xmlaligns with the.gitignoreupdates to prevent these files from being committed.application/datamanager/pyproject.toml (1)
12-12: Verify the new self-dependency.
The service is adding"datamanager"to its own dependencies, which can create a cyclic or redundant requirement. If this is intended for workspace linking, confirm that it's correctly resolved by the build tool; otherwise, remove it.Dockerfile.test (3)
1-2: Confirm base image and tool installation.
Usingpython:3.13and copyinguvfromghcr.io/astral-sh/uv:latestassumes the3.13tag exists on Docker Hub and that theuvbinary is compatible. Please verify the availability ofpython:3.13and test theuvbinary in this environment.
4-4: Root directory for pytest set.
ENV PYTEST_ADDOPTS="--rootdir=/tests"aligns with/testsas the mounted working directory. Good for isolating test runs.
6-6: WORKDIR matches mount point.
Using/testsas both WORKDIR and mount path simplifies bind mounting incompose.yaml. No issues here.application/positionmanager/Dockerfile (4)
4-5: SetPYTHONPATHfor module resolution.
DefiningENV PYTHONPATH=/app/srcensuresuvpicks up thesrcdirectory. This follows the pattern in thedatamanagerservice. Well done.
9-9: Sync only production dependencies.
Usinguv sync --no-devis correct for a production build to exclude dev tools.
11-11: Copy application source directory.
Changing from service-specific folders to a consistent./srcdirectory promotes uniformity across services.
15-15: Entrypoint adjusted foruvCLI.
Updating the module path topositionmanager.main:applicationwith--app-dir srcaligns with the workspace layout. Looks correct.compose.yaml (1)
2-7: Verify thetestsservice build configuration
Ensure thecontext: .anddockerfile: Dockerfile.testaccurately reflect the repository layout and thatDockerfile.testis located at the repo root.application/datamanager/Dockerfile (5)
4-4: SetPYTHONPATHfor application imports
UsingENV PYTHONPATH=/app/srcaligns with the new directory structure and entrypoint flags — this change looks correct.
7-8: Install required runtime dependencies
Addingcurlalongsidelibgomp1and cleaning up apt lists follows best practices for minimal, secure images.
13-13: Synchronize production dependencies only
Runninguv sync --no-devensures dev dependencies are excluded from the final image, improving security and reducing size.
15-15: Adjust source copy location
Copying./srcexplicitly ensures that only application code (and not extraneous files) is included in the image.
19-19: Expose application port update
Updating from port 8000 to 8080 matches conventions used across services and aligns with the Compose definitions..github/workflows/test.yaml (2)
14-15: Simplified runner configuration
Hardcodingruns-on: ubuntu-latestaligns with the containerized test approach and reduces maintenance overhead.
24-24: Use consistent test invocation command
Invokingmise tasks run python:testdelegates testing to the Docker Compose service, ensuring parity between local and CI runs.
chrisaddy
force-pushed
the
nix-ci
branch
3 times, most recently
from
800d394 to
85ce3b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Dockerfile.test (1)
1-1: Ensure Python base image aligns with project compatibility and optimizes image size.The project’s
pyproject.tomlspecifies>=3.12, but this Dockerfile uses the fullpython:3.13image. To both guarantee compatibility and reduce image size/build time, consider pinning to a specific patch version and using a slim variant. For example:- FROM python:3.13 + FROM python:3.13-slimOr, if you prefer to test on the minimum supported version:
- FROM python:3.13 + FROM python:3.12-slim
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.mise.toml(2 hunks)Dockerfile.test(1 hunks)application/datamanager/Dockerfile(1 hunks)application/datamanager/pyproject.toml(1 hunks)compose.yaml(1 hunks)pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- application/datamanager/Dockerfile
- pyproject.toml
- compose.yaml
- application/datamanager/pyproject.toml
- .mise.toml
🧰 Additional context used
🪛 Hadolint (2.12.0)
Dockerfile.test
[error] 8-8: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
🔇 Additional comments (2)
Dockerfile.test (2)
4-6: ENV and WORKDIR configuration is appropriate.Setting
PYTEST_ADDOPTSto--rootdir=/testsand switching the working directory to/testsaligns correctly with how the Compose service mounts the source for testing.
9-9: Dependency synchronization step is correctly configured.Running
uv sync --all-groupsensures all dependency groups (including dev/test) are installed for coverage and test execution.
forstmeier
left a comment
forstmeier
left a comment
There was a problem hiding this comment.
Just some questions to address otherwise it looks good.
| # name: Run Python tests (${{ matrix.os }}) | ||
| # runs-on: ${{ matrix.os }} | ||
| # strategy: | ||
| # matrix: | ||
| # os: | ||
| # - ubuntu-latest | ||
| # - macos-latest |
There was a problem hiding this comment.
These comments can be removed.
| version = "0.1.0" | ||
| description = "Data management service" | ||
| requires-python = ">=3.13" | ||
| requires-python = ">=3.12" |
There was a problem hiding this comment.
No issues just checking we're okay downgrading to 3.12.
| "duckdb>=1.2.2", | ||
| "polars>=1.29.0", | ||
| "pyarrow>=20.0.0", | ||
| "datamanager" |
There was a problem hiding this comment.
Is this actually needed now?
| @@ -0,0 +1,17 @@ | |||
|
|
|||
There was a problem hiding this comment.
Removing leading empty line.
| version = "0.1.0" | ||
| description = "Open source quantitative hedge fund 🍊" | ||
| requires-python = ">=3.13" | ||
| requires-python = ">=3.12" |
| @@ -0,0 +1,9 @@ | |||
| FROM python:3.13 | |||
There was a problem hiding this comment.
This is version 3.13 whereas you've set the pyproject.toml to 3.12. Intentional?
…compose.yaml Fix YAML indentation in compose file
…compose.yaml Fix YAML indentation in compose file Merge pull request #552 from pocketsizefund/codex/fix-indentation-in-compose.yaml Fix YAML indentation in compose file
2 participants
This pull request introduces significant updates to the CI/CD pipeline, Python dependency management, and Docker configurations to improve development workflows and testing processes. The key changes include switching to Docker-based testing, updating dependency management, and refining Dockerfile configurations for multiple services.
CI/CD and Testing Improvements:
.github/workflows/test.yamlto simplify the CI workflow by removing the matrix strategy and hardcodingubuntu-latestas the runner. This change also removes unused steps for installing dependencies.Dockerfile.testfor running tests in a containerized environment, including dependency installation viauv sync --all-groups.testsservice incompose.yamlfor running Python tests with coverage reporting using Docker Compose.Dependency Management:
.mise.tomlto usedocker compose run testsfor running Python tests and simplified thepython:installtask by removing the--all-packagesflag. [1] [2]pyproject.tomlto define adevdependency group for development tools and added configuration forpytestandcoverage.requires-pythonversion inpyproject.tomlfrom>=3.13to>=3.12for broader compatibility.Dockerfile Updates:
application/datamanager/Dockerfileandapplication/positionmanager/Dockerfileto align their configurations, including settingPYTHONPATH, copying source files to/app/src, and usinguv sync --no-devfor dependency installation. [1] [2]ENTRYPOINTcommands to include the--app-dirflag for better structure. [1] [2]Dependency Additions:
datamanagerdependency to theapplication/datamanager/pyproject.tomlfile.Summary by CodeRabbit
New Features
Improvements
Chores