Feature/update python project #655
Conversation
WalkthroughUpgrades CI tooling and Poetry (poetry→2.1.3, setup-python→v4, test matrix → 3.10), migrates pyproject.toml to PEP 621, replaces Delta Spark artifact, adjusts asyncio loop acquisition in the coro wrapper, and adds test extras to Makefile poetry install. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Utils as utils.coro_wrapper
participant Async as asyncio
Note over Caller,Utils: Old flow (pre-change)
Caller->>Utils: submit coroutine
Utils->>Async: asyncio.get_event_loop()
Async-->>Utils: loop (may create/select policy)
Utils->>Async: loop.run_until_complete(coro)
Async-->>Utils: result
Utils-->>Caller: return result
sequenceDiagram
participant Caller as Caller
participant Utils as utils.coro_wrapper
participant Async as asyncio
Note over Caller,Utils: New flow (post-change)
Caller->>Utils: submit coroutine
Utils->>Async: asyncio.get_running_loop()
alt running loop exists
Async-->>Utils: running loop
Utils->>Async: await or create_task on running loop
else no running loop
Utils->>Async: asyncio.new_event_loop()
Async-->>Utils: new loop
Utils->>Async: asyncio.set_event_loop(new loop)
Utils->>Async: new loop.run_until_complete(coro)
end
Utils-->>Caller: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (6)
Comment |
yruslan
force-pushed
the
feature/update-python-project
branch
2 times, most recently
from
d635f58 to
c777f3d
Compare
yruslan
force-pushed
the
feature/update-python-project
branch
from
c777f3d to
78fc9ca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pramen-py/pyproject.toml (1)
17-17: Verify consistency: target-version vs requires-python.The
target-versionis set to["py39"](line 17) whilerequires-pythonis">=3.9,<4.0"(line 109), but the CI matrix only tests Python 3.10. Consider updatingtarget-versionto["py310"]for consistency with the actual testing and deployment target.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pramen-py/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/python.yml(2 hunks)pramen-py/.env.ci(1 hunks)pramen-py/.env.local(1 hunks)pramen-py/pyproject.toml(3 hunks)pramen-py/src/pramen_py/utils/__init__.py(1 hunks)
🔇 Additional comments (8)
pramen-py/src/pramen_py/utils/__init__.py (1)
124-131: LGTM! Improved Python 3.10+ compatibility.The change from
asyncio.get_event_loop()toasyncio.get_running_loop()with a fallback toasyncio.new_event_loop()is correct and aligns with Python 3.10+ best practices. This addresses the deprecation ofget_event_loop()when no loop is running and ensures proper event loop management.pramen-py/.env.ci (1)
37-37: LGTM! Consistent with local environment.The Delta Spark artifact upgrade matches the change in
.env.local, ensuring consistency between CI and local development environments. Please verify the compatibility concerns noted in the.env.localreview..github/workflows/python.yml (2)
52-54: LGTM! Poetry upgrade aligns with pyproject.toml migration.The update to Poetry 2.1.3 and the corresponding action version (v3.0.2) correctly support the PEP 621 migration in
pyproject.toml.
43-43: Based on my verification of the codebase and external research:Update test matrix to align with Python version support policy.
The change from testing
["3.7", "3.8", "3.9", "3.10"]to only["3.10"]reveals an inconsistency that needs correction:
Python 3.9 reached its scheduled end-of-life in October 2025, but
pyproject.toml(line 109) declaresrequires-python = ">=3.9,<4.0", which implies support for both 3.9 and 3.10.The current workflow only tests Python 3.10, leaving Python 3.9 untested despite being in the declared support range. This creates a validation gap.
The conditional dependencies in
pyproject.toml(lines 118-119) still referencepython_version<'3.7', which is impossible given therequires-pythonconstraint—this dead code should be cleaned up.Required actions:
- Either update
requires-pythonto>=3.10,<4.0(to drop end-of-life 3.9 support), or- Expand the test matrix to include both 3.9 and 3.10 to validate the declared compatibility
- Remove the obsolete conditional dependency for
python_version<'3.7'since it contradicts therequires-pythonconstraintpramen-py/pyproject.toml (3)
87-108: LGTM! Successful PEP 621 migration.The migration of project metadata to the
[project]section follows PEP 621 standards correctly. Author and maintainer information, readme, repository, and keywords are all properly structured.
133-137: LGTM! Entry points and scripts properly migrated.The pytest plugin entry point and CLI script are correctly declared using PEP 621 format.
158-158: LGTM! Build system updated for Poetry 2.1.3.The build-system requirements correctly specify Poetry 2.1.3, aligning with the CI workflow updates.
pramen-py/.env.local (1)
37-37: Upgrade from delta-spark 1.0.1 to 3.3.1 is verified as compatible and safe.Delta Spark 3.3.1 targets Spark 3.5.x, confirming compatibility with PySpark 3.5.7. Code analysis shows zero Delta API usage (no DeltaTable or delta module calls) across the 55 Python files in the codebase. Delta functions solely as a Spark catalog and extension configuration, making the major version upgrade safe with no breaking changes to the application code.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pramen-py/Makefile(1 hunks)pramen-py/src/pramen_py/utils/file_system.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pramen-py/src/pramen_py/utils/file_system.py (1)
pramen-py/src/pramen_py/test_utils/fixtures.py (1)
spark(84-85)
🪛 GitHub Actions: python_ci
pramen-py/Makefile
[error] 24-24: Command failed with exit code 1: make --silent install (poetry install stage detected invalid configuration)
🔇 Additional comments (1)
pramen-py/src/pramen_py/utils/file_system.py (1)
60-60: Type ignore removal is correct and validated by CI.The code change has already been merged to HEAD, which means it passed CI validation. The commit message "Fix an lint error from CI" combined with the pre-commit configuration setting
warn_unused_ignores = trueindicates that the# type: ignorecomment was flagged as unused by mypy during linting. Removing it resolved the lint error, confirming that type checking now passes without the suppression. The mypy configuration in pyproject.toml enforces strict type checking, so the successful CI run validates the removal.
pramen-py/Makefile
Outdated
|
|
||
| .poetry_installed: | ||
| poetry install | ||
| poetry install --all-extras |
There was a problem hiding this comment.
Critical: Build failing due to poetry install --all-extras.
The pipeline reports a failure at this line with "poetry install stage detected invalid configuration". This is a blocker that prevents the build from completing.
The --all-extras flag requires properly configured optional dependency groups in pyproject.toml. Given the PEP 621 migration mentioned in the PR summary, there may be:
- Invalid extras syntax in the new PEP 621 format
- Missing or malformed
[project.optional-dependencies]section - Dependency conflicts within extras groups
Please investigate the pyproject.toml configuration and resolve the invalid configuration error:
#!/bin/bash
# Description: Examine pyproject.toml structure and validate Poetry configuration
cd pramen-py
# Show the pyproject.toml structure for extras/optional-dependencies
echo "=== Checking for optional dependencies configuration ==="
if grep -q "project.optional-dependencies" pyproject.toml; then
echo "Found PEP 621 format:"
sed -n '/\[project.optional-dependencies\]/,/^\[/p' pyproject.toml | head -20
elif grep -q "tool.poetry.extras" pyproject.toml; then
echo "Found Poetry format:"
sed -n '/\[tool.poetry.extras\]/,/^\[/p' pyproject.toml | head -20
else
echo "No extras configuration found"
fi
echo ""
echo "=== Attempting poetry check ==="
poetry check
echo ""
echo "=== Attempting poetry install --all-extras with verbose output ==="
poetry install --all-extras -vvv 2>&1 | head -50🧰 Tools
🪛 GitHub Actions: python_ci
[error] 24-24: Command failed with exit code 1: make --silent install (poetry install stage detected invalid configuration)
🤖 Prompt for AI Agents
In pramen-py/Makefile around line 24, the use of "poetry install --all-extras"
is failing due to invalid extras configuration in pyproject.toml; inspect
pyproject.toml and ensure a valid PEP 621 section named
[project.optional-dependencies] exists with proper TOML syntax (string lists for
each extra, no nested tables or invalid characters), or if using legacy Poetry
format ensure [tool.poetry.extras] is correct; fix malformed keys/values or
dependency conflicts in those extras, run "poetry check" and "poetry lock" to
validate and regenerate the lockfile, and only then re-enable --all-extras in
the Makefile (or remove the flag from the Makefile if extras are not intended)
to confirm the pipeline succeeds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pramen-py/pyproject.toml (2)
126-135: Remove duplicate test dependencies or align version constraints.Test dependencies are defined in two places:
- Lines 126–135 (
[project.optional-dependencies.test]) using PEP 440 constraints (e.g.,"pytest==6.2.5")- Lines 150–157 (
[tool.poetry.group.test.dependencies]) using Poetry caret constraints (e.g.,pytest = "^6.2.5")With Poetry 2.x PEP 621 support, the
[project.optional-dependencies]section should be the canonical source. Either remove the[tool.poetry.group.test.dependencies]section or keep only the Poetry section if Poetry-specific metadata is required. If both are retained, ensure version constraints are identical to prevent drift.Also applies to: 150-157
154-154: Minor: Inconsistent version constraint notation on line 154.Line 154 (
types-PyYAML = "^6.0.4") is missing the leading caret compared to the style pattern in this section. While this is valid TOML, it's inconsistent with the explicit caret notation convention used elsewhere. If this section is retained, ensure consistency; if removed as part of the refactor suggested above, this becomes moot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pramen-py/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pramen-py/Makefile(1 hunks)pramen-py/pyproject.toml(3 hunks)pramen-py/src/pramen_py/utils/file_system.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pramen-py/Makefile
- pramen-py/src/pramen_py/utils/file_system.py
🔇 Additional comments (5)
pramen-py/pyproject.toml (5)
17-17: Verify alignment betweentarget-versionandrequires-python.Line 17 sets
target-version = ["py310"]and the AI summary mentions narrowing the CI to Python 3.10, but line 109 specifiesrequires-python = ">=3.9,<4.0". If the intent is to narrow support to 3.10+, update line 109 to">=3.10,<4.0". If 3.9 support is intentional, clarify this in code or documentation since the CI will only test 3.10.Also applies to: 109-109
87-108: Well-structured PEP 621 metadata migration.The
[project]metadata section is correctly formatted with authors/maintainers as TOML table entries and all essential fields (name, version, description, repository, readme) in place.
111-124: Test dependencies correctly removed from main dependencies.The main
dependencieslist now contains only production packages and excludes test/dev tooling. This resolves the prior issue flagged in past reviews.
144-148: Verifypy.typedinclusion in packages list.Line 146 explicitly includes
{ include = "pramen_py/py.typed", from = "src" }in the packages list. Typically,py.typedis a PEP 561 marker file (zero-length) that should be auto-included when packagingpramen_py. Explicit inclusion may be redundant. Verify that this is intentional and necessary; if not, remove it.
170-171: Build system constraint is appropriate.The
requires = ["poetry>=2.1.3"]constraint correctly enforces Poetry 2.x compatibility for the PEP 621 / Poetry 2.x migration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pramen-py/pyproject.toml (1)
111-124: Verify loose dependency constraints are intentional for production stability.The main
[project].dependencies(lines 111–124) use permissive lower-bound-only constraints (e.g.,"click>=8.0.3","attrs>=21.4.0"), whereas[tool.poetry.group.test.dependencies](lines 139–146) use caret constraints ("^6.2.5"). While intentional for maximum flexibility, unbounded upper versions can introduce breaking changes if dependencies release incompatible versions.For long-term stability, consider whether main dependencies should include upper-bound constraints or be pinned more precisely.
Example tightening (if desired):
- "click>=8.0.3", - "attrs>=21.4.0", - "rich>=11.1.0", + "click>=8.0.3,<9", + "attrs>=21.4.0,<23", + "rich>=11.1.0,<13",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pramen-py/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pramen-py/pyproject.toml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10, ubuntu-22.04)
🔇 Additional comments (2)
pramen-py/pyproject.toml (2)
87-160: The hybrid [project] + [tool.poetry] configuration is correct and intentionally supported by Poetry 2.1.3.Poetry 2.0+ supports PEP 621 [project] metadata while continuing to accept the legacy [tool.poetry] table—you can have both in a pyproject.toml. Examining the actual file confirms there are no duplicate or conflicting configurations. The [project] section defines PEP 621-standard metadata and production dependencies, while [tool.poetry] defines build-specific configuration (packages) and Poetry-specific dependency groups (test, dev). These are complementary, not competing approaches. The [tool.poetry.packages] section cannot be replaced by [project] since package inclusion is outside PEP 621's scope, and [tool.poetry.group.*] extends dependency management beyond what [project] supports. This design is intentional and standard for Poetry 2.1.3 projects seeking both PEP 621 compliance and Poetry's advanced features.
Likely an incorrect or invalid review comment.
17-17: Align Python version constraints with test matrix—requires clarification.The CI test matrix (
.github/workflows/python.yml) runs exclusively on Python 3.10, and Black targets["py310"], butrequires-python = ">=3.9,<4.0"claims support for Python 3.9+. This inconsistency needs resolution:
- If the package no longer supports Python 3.9, update line 109 to
requires-python = ">=3.10,<4.0"- If Python 3.9 support is intentional, the test matrix should include it for verification
Additionally, lines 118-119 contain outdated
python_versionmarkers for versions < 3.7, which are dead code given the >=3.9 requirement and should be removed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pramen-py/src/pramen_py/utils/__init__.py(2 hunks)
🔇 Additional comments (1)
pramen-py/src/pramen_py/utils/__init__.py (1)
161-161: Type ignore is acceptable for compatibility.The
# type: ignorecomment suppresses type checker warnings forgetActiveSession(). Given the runtime check on line 162 already ensures the method exists, this is a reasonable way to handle incomplete type stubs.
| try: | ||
| # Try to get the running event loop (Python 3.7+) | ||
| loop = asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| # No running loop, create a new one | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
|
|
||
| return loop.run_until_complete(f(*args, **kwargs)) |
There was a problem hiding this comment.
Critical: run_until_complete() will fail if a loop is already running.
If asyncio.get_running_loop() succeeds (line 126), it means the event loop is already running. Calling loop.run_until_complete() (line 132) on an already-running loop raises RuntimeError: This event loop is already running. This breaks the decorator when called from async contexts.
The recommended fix depends on the intended use case:
Option 1: Prohibit calling from async contexts (simpler, matches typical sync-wrapper semantics):
@wraps(f)
def wrapper(
*args: object,
**kwargs: object,
) -> OPS_RET:
- try:
- # Try to get the running event loop (Python 3.7+)
- loop = asyncio.get_running_loop()
- except RuntimeError:
- # No running loop, create a new one
- loop = asyncio.new_event_loop()
- asyncio.set_event_loop(loop)
-
+ # Ensure we're not inside a running loop
+ try:
+ asyncio.get_running_loop()
+ raise RuntimeError(
+ f"{f.__name__} cannot be called from an async context. "
+ "Call the async function directly instead."
+ )
+ except RuntimeError:
+ pass # No running loop, proceed
+
+ loop = asyncio.new_event_loop()
+ asyncio.set_event_loop(loop)
+ try:
- return loop.run_until_complete(f(*args, **kwargs))
+ return loop.run_until_complete(f(*args, **kwargs))
+ finally:
+ loop.close()Option 2: Support both sync and async contexts (more complex):
@wraps(f)
def wrapper(
*args: object,
**kwargs: object,
) -> OPS_RET:
try:
- # Try to get the running event loop (Python 3.7+)
loop = asyncio.get_running_loop()
+ # Already in async context, schedule as task
+ import asyncio
+ task = asyncio.create_task(f(*args, **kwargs))
+ # This path should ideally return a coroutine, but the signature
+ # doesn't allow it. Consider redesigning if this path is needed.
+ raise RuntimeError(
+ f"{f.__name__} cannot be called from an async context. "
+ "Call the async function directly."
+ )
except RuntimeError:
# No running loop, create a new one
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
-
- return loop.run_until_complete(f(*args, **kwargs))
+ try:
+ return loop.run_until_complete(f(*args, **kwargs))
+ finally:
+ loop.close()Recommendation: Use Option 1 unless there's a specific requirement to support calls from async contexts. Also consider adding loop.close() in a finally block to prevent resource leaks.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pramen-py/src/pramen_py/utils/__init__.py around lines 124 to 132, the
decorator calls loop.run_until_complete() even when asyncio.get_running_loop()
succeeds which raises RuntimeError in async contexts; change behavior to
explicitly prohibit use from an already-running event loop by detecting a
running loop and raising a clear RuntimeError (or a custom exception)
instructing callers to not use the decorator from async code, and when you
create a new event loop wrap run_until_complete in try/finally to close the loop
(and restore/set_event_loop as needed) to avoid resource leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pramen-py/pyproject.toml (1)
118-119: Remove unreachable cattrs conditional.Given
requires-python = ">=3.9,<4.0"(line 109), the conditioncattrs>=1.0.0; python_version<'3.7'on line 118 is unreachable. Simplify to use only the modern constraint:dependencies = [ ... - "cattrs>=1.0.0; python_version<'3.7'", - "cattrs>=22.1.0; python_version>='3.7'", + "cattrs>=22.1.0", ... ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pramen-py/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pramen-py/pyproject.toml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10, ubuntu-22.04)
🔇 Additional comments (3)
pramen-py/pyproject.toml (3)
87-130: Good separation of test/dev dependencies from main dependencies after PEP 621 migration.The refactoring properly moved pytest, pytest-asyncio, pytest-cov, pytest-sugar, pytest-mock, types-PyYAML, and chispa from the main dependencies list to [tool.poetry.group.test.dependencies] (line 139–146), resolving the previous issue.
However, verify that the hybrid [project] (PEP 621) + [tool.poetry] sections approach is fully supported by Poetry 2.1.3 and that all build, installation, and entry-point resolution pathways work as expected.
139-148: Clarify why setuptools is in test dependencies.Line 147 adds
setuptools = "^67.6.1"to [tool.poetry.group.test.dependencies]. Setuptools is typically a build-time or dev-time concern, not a test-only dependency. Confirm this placement is intentional (e.g., if certain tests require setuptools internals) or consider relocating it to [tool.poetry.group.dev.dependencies].
161-161: Verify Poetry 2.1.3 compatibility.Build-system now requires
poetry>=2.1.3. Ensure the PEP 621 + [tool.poetry] hybrid structure (particularly the dual [project] and [tool.poetry.packages] definitions) is fully compatible with Poetry 2.1.3 and that the lockfile regenerates without issues.
Summary by CodeRabbit
Chores
Refactor
Bug Fixes