Conversation
WalkthroughThe updates introduce new dependencies and type-checking rules in the project configuration, adjust type annotations to allow Changes
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 (
|
Graphite Automations"Assign author to pull request" took an action on this PR • (05/28/25)1 assignee was added to this PR based on John Forstmeier's automation. |
![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
🧹 Nitpick comments (1)
pyproject.toml (1)
68-69: Consider the implications of ignoring unresolved imports.The
unresolved-import = "ignore"configuration may hide legitimate import issues. While this can be helpful during development or when dealing with dynamic imports, it could mask real problems.Consider being more specific about which imports to ignore, or document why this broad ignore rule is necessary. You might want to use more targeted ignores in the code instead:
[tool.ty.rules] -unresolved-import = "ignore" +# Consider removing this and using targeted # ty: ignore comments instead
📜 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 (5)
.mise.toml(2 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)infrastructure/project.py(1 hunks)pyproject.toml(2 hunks)
🔇 Additional comments (5)
pyproject.toml (1)
7-10:Details
✅ Verification successful
Verify new dependency versions and security.
The added dependencies support infrastructure and workflow management, which aligns with the project's needs. However, ensure these specific versions are current and free from known vulnerabilities.
🏁 Script executed:
#!/bin/bash # Description: Check for the latest versions and security advisories for new dependencies echo "Checking flytekit..." curl -s https://pypi.org/pypi/flytekit/json | jq -r '.info.version' echo "Checking pulumi-docker-build..." curl -s https://pypi.org/pypi/pulumi-docker-build/json | jq -r '.info.version' echo "Checking pulumi-gcp..." curl -s https://pypi.org/pypi/pulumi-gcp/json | jq -r '.info.version' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: PIP, package: "flytekit") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: PIP, package: "pulumi-docker-build") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: PIP, package: "pulumi-gcp") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1324
Dependencies are up-to-date and have no known vulnerabilities.
Verified on PyPI that:
- flytekit 1.15.4 is the latest release
- pulumi-docker-build 0.0.12 is the latest release
- pulumi-gcp has moved to 8.32.1 (your constraint ≥8.32.0 covers it)
GitHub’s security advisories show no reported vulnerabilities for these packages. No further action required.
infrastructure/project.py (1)
57-59:Details
✅ Verification successful
Investigate if the type ignore directive is necessary.
The
# ty: ignore[missing-argument]suggests a potential typing issue with the IAMMember constructor or lambda function. While type ignores can be useful, it's better to address the root cause when possible.
🏁 Script executed:
#!/bin/bash # Description: Check the IAMMember constructor signature and usage patterns # Look for IAMMember class definition and constructor ast-grep --pattern $'class IAMMember { $$$ def __init__($$$) { $$$ } $$$ }' # Check for other IAMMember usages in the codebase rg -A 10 -B 5 "IAMMember\(" # Look for similar patterns with member parameter rg -A 5 -B 5 "member.*apply"Length of output: 1294
Keep the
# ty: ignore[missing-argument]hereThe ignore suppresses a false-positive from Pyright/Pylance on Pulumi’s
Output.applysignature (the type stubs declare extra parameters that aren’t needed in practice). No changes are required..mise.toml (2)
16-16: Good practice using uvx for tool execution.Using
uvx vultureinstead of directvultureis a good practice as it manages tool dependencies in isolation and ensures consistent tool versions across environments.
29-29: Type checking integration looks good.Adding
uvx ty checkto the lint task properly integrates the type checking workflow. This works well with the[tool.ty.rules]configuration added inpyproject.toml.application/datamanager/src/datamanager/config.py (1)
8-8: Type annotation improvement is correct.Changing
api_key: strtostr | Noneaccurately reflects thatos.getenv("POLYGON_API_KEY")can returnNonewhen the environment variable is not set.
rebasing
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: 1
🔭 Outside diff range comments (1)
application/datamanager/src/datamanager/config.py (1)
20-22:⚠️ Potential issueRemove duplicate
daily_bars_pathcomputed field.You have two
@computed_fieldmethods with the same namedaily_bars_path. This will cause the second definition (lines 20-22) to override the first one (lines 14-18), losing the important None validation logic.Remove the duplicate method:
- @computed_field - def daily_bars_path(self) -> str: - return f"gs://{self.name}/equity/bars/"Keep only the first implementation (lines 14-18) which properly handles the case when
self.nameisNone.🧰 Tools
🪛 Ruff (0.11.9)
21-21: Redefinition of unused
daily_bars_pathfrom line 15(F811)
♻️ Duplicate comments (1)
application/positionmanager/src/positionmanager/clients.py (1)
63-63: Add default value for optional parameter.The
datamanager_base_urlparameter is typed asstr | Nonebut lacks a default value, making it not truly optional.- def __init__(self, datamanager_base_url: str | None): + def __init__(self, datamanager_base_url: str | None = None):
📜 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 (5)
.mise.toml(2 hunks)application/datamanager/src/datamanager/config.py(1 hunks)application/positionmanager/src/positionmanager/clients.py(2 hunks)infrastructure/project.py(1 hunks)pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- infrastructure/project.py
- .mise.toml
- pyproject.toml
🧰 Additional context used
🪛 Ruff (0.11.9)
application/positionmanager/src/positionmanager/clients.py
15-16: SyntaxError: Expected ')', found newline
16-16: SyntaxError: Invalid annotated assignment target
17-17: SyntaxError: Invalid annotated assignment target
18-18: SyntaxError: Invalid annotated assignment target
19-19: SyntaxError: Invalid annotated assignment target
20-20: SyntaxError: Unexpected indentation
21-21: SyntaxError: unindent does not match any outer indentation level
21-21: SyntaxError: Expected a statement
21-21: SyntaxError: Expected a statement
21-21: SyntaxError: Invalid annotated assignment target
21-22: SyntaxError: Expected an expression
🔇 Additional comments (2)
application/datamanager/src/datamanager/config.py (2)
8-8: Good improvement to type safety.The change from
strtostr | Nonecorrectly reflects thatos.getenv()can returnNonewhen the environment variable is not set. This enhances type safety and makes the nullable nature explicit.
14-18:⚠️ Potential issueMissing
namefield in Bucket class.The
daily_bars_pathcomputed field referencesself.name, but there's nonamefield defined in theBucketclass. This will cause anAttributeErrorat runtime.Add the missing field to the
Bucketclass:class Bucket(BaseModel): + name: str | None = Field(default=os.getenv("DATA_BUCKET")) + @computed_field def daily_bars_path(self) -> str:Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fixes from coderabbit
2 participants
This pull request introduces several updates across configuration files, Python models, and dependencies to enhance functionality, improve type safety, and update tooling. Key changes include adding new dependencies, improving type annotations, and updating linting and testing configurations.
Configuration and Tooling Updates:
.mise.toml: Addeduvxwrapper forvultureandty checkto improve linting and type-checking workflows. [1] [2]pyproject.toml: Added new dependencies (flytekit,pulumi-docker-build,pulumi-gcp) and configuredtyrules to ignore unresolved imports. [1] [2]Type Annotations and Model Updates:
application/datamanager/src/datamanager/config.py: UpdatedPolygonandBucketmodels to useOptionaltype hints (str | None) for environment variable-based fields, improving type safety.application/positionmanager/src/positionmanager/clients.py: UpdatedAlpacaClientandDataClientconstructors to useOptionaltype hints (str | None) for parameters, ensuring better handling of optional values. [1] [2]Codebase Maintenance:
infrastructure/project.py: Added a# ty: ignore[missing-argument]comment to suppress type-checking warnings for a lambda function.Summary by CodeRabbit
New Features
Style
Chores