Skip to content

Conversation

@helmut-hoffer-von-ankershoffen
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


PR Review: Python 3.14 Support

Reviewing PR #321 for Python 3.14.x support...

Todo List

  • Read CLAUDE.md documentation files
  • Review commit message format
  • Examine changed files for compatibility issues
  • Install dependencies
  • Check for test marker compliance
  • Run linting checks
  • Run unit tests
  • Verify documentation updates needed
  • Provide comprehensive feedback

Initial Findings:

  • ✅ Commit message follows conventional commit format: feat(core): support for python 3.14.x
  • ⚠️ BLOCKING: .python-version contains 3.14.01 - should be 3.14.0 (no leading zero in patch)
  • pyproject.toml correctly updated Python classifiers and requires-python
  • noxfile.py includes 3.14 in test matrix: ["3.11.14", "3.12.12", "3.13.10", PYTHON_VERSION]
  • ✅ Code changes migrated from google-crc32c to crc32c package (Python 3.14 compatibility)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude finished @helmut-hoffer-von-ankershoffen's task —— View job


PR Review: Python 3.14 Support

Reviewing PR #321 for Python 3.14.x support...

Todo List

  • Read CLAUDE.md documentation files
  • Review commit message format
  • Examine .python-version format
  • Verify complete migration from google-crc32c to crc32c
  • Install dependencies and sync environment
  • Check for test marker compliance
  • Run linting checks
  • Identify all issues
  • Provide comprehensive feedback

Summary

This PR adds Python 3.14.x support with dependency updates. Linting passes ✅, test markers are compliant ✅, but there is 1 BLOCKING issue that must be fixed before merge.


🚨 BLOCKING ISSUE

Incomplete Migration from google-crc32c to crc32c

Severity: CRITICAL
Location: src/aignostics/application/_service.py:12 and :374

Problem:
The migration from google-crc32c to crc32c package is incomplete. While pyproject.toml correctly removes google-crc32c (line 99) and adds crc32c (line 99), the code in _service.py still imports and uses the old API:

# Line 12
import google_crc32c

# Lines 373-378
# Generate CRC32C checksum with google_crc32c and encode as base64
hash_sum = google_crc32c.Checksum()  # type: ignore[no-untyped-call]
with file_path.open("rb") as f:
    while chunk := f.read(1024):
        hash_sum.update(chunk)  # type: ignore[no-untyped-call]
checksum = str(base64.b64encode(hash_sum.digest()), "UTF-8")  # type: ignore[no-untyped-call]

Why this didn't fail:
Currently, google-crc32c is still present as a transitive dependency of google-cloud-storage:

$ uv pip show google-crc32c
Required-by: google-cloud-storage, google-resumable-media

However, relying on transitive dependencies is fragile and violates best practices:

  1. Implicit dependency: Code depends on a package not explicitly declared
  2. Future breakage risk: If google-cloud-storage removes this dependency, the code breaks
  3. Inconsistent migration: Two other files were correctly migrated (_download.py, _utils.py)

How to reproduce:

grep -rn "google_crc32c" src/ --include="*.py"

Correct fix (pattern from _download.py:168-173 and _utils.py:171-176):

# Change import at line 12
import crc32c

# Change lines 373-378
# Generate CRC32C checksum with crc32c and encode as base64
checksum = crc32c.CRC32CHash()
with file_path.open("rb") as f:
    while chunk := f.read(1024):
        checksum.update(chunk)
checksum_b64 = base64.b64encode(checksum.digest()).decode("UTF-8")

Verify fix:

# Ensure no references remain
grep -rn "google_crc32c\|google-crc32c" src/ --include="*.py"

# Run linting
make lint

# Run unit tests
make test_unit

✅ What's Good

1. Commit Messages

Both commits follow conventional commit format correctly:

feat(core): support for python 3.14.x

2. Dependency Updates

  • pyproject.toml: Correctly updated requires-python to >=3.11, <3.15
  • ✅ Added Python 3.14 classifier
  • ✅ Updated pywin32 to >=311,<312 for Windows compatibility
  • ✅ Updated duckdb to >=0.14.2,<=1.5 for Python 3.14 compatibility
  • ✅ Added lxml>=6.0.2 to override-dependencies for Python 3.14 pre-built wheels
  • ✅ Migrated from google-crc32c to crc32c (partial - see blocking issue)

3. Build Configuration

  • .python-version: Updated to 3.14.1
  • Dockerfile: Updated base image to python:3.14.1-slim-trixie
  • noxfile.py: Test matrix includes PYTHON_VERSION from .python-version

4. Successful Migrations (2/3 files)

  • src/aignostics/application/_download.py: Correctly uses crc32c.CRC32CHash()
  • src/aignostics/platform/_utils.py: Correctly uses crc32c.CRC32CHash()

5. Code Quality

  • Linting passed: Ruff format, ruff check, pyright, mypy all successful
  • Test markers compliant: No unmarked tests found (643 tests collected with proper markers)
  • Type hints preserved: All type annotations intact after migration

📋 Recommendations (Non-Blocking)

1. CI/CD Validation

The CI workflows dynamically use .python-version via uv, so Python 3.14 will be tested automatically. However, verify:

  • CI passes on all platforms (ubuntu-latest, ubuntu-24.04-arm, macos-latest, macos-13, windows-latest)
  • Native builds succeed for all 6 target platforms
  • Docker image builds successfully with Python 3.14

2. Documentation Updates

Consider updating:

  • CHANGELOG.md with breaking Python version support changes
  • README.md if Python version requirements are documented
  • Any user-facing docs mentioning Python compatibility

3. Test Coverage

Since generate_metadata_from_source_directory() wasn't covered by tests (which is why the incomplete migration wasn't caught), consider:

  • Adding integration tests for this function
  • Verifying coverage for CRC32C checksum functionality

🔧 How to Fix

Single command to fix the blocking issue:

# Update the import and usage in _service.py
sed -i 's/import google_crc32c/import crc32c/' src/aignostics/application/_service.py
sed -i 's/google_crc32c\.Checksum()/crc32c.CRC32CHash()/' src/aignostics/application/_service.py
sed -i 's/# Generate CRC32C checksum with google_crc32c/# Generate CRC32C checksum with crc32c/' src/aignostics/application/_service.py

Or manually edit src/aignostics/application/_service.py:

  1. Line 12: Change import google_crc32cimport crc32c
  2. Line 373: Change comment to reference crc32c instead of google_crc32c
  3. Line 374: Change google_crc32c.Checksum()crc32c.CRC32CHash()
  4. Lines 377-378: Change hash_sum variable name to checksum for consistency
  5. Line 378: Simplify to base64.b64encode(checksum.digest()).decode("UTF-8")

Verify the fix:

make lint && make test_unit

📊 Impact Assessment

Scope: Core functionality (application run submission with source directory metadata generation)
Severity: HIGH - Code depends on undeclared transitive dependency
Likelihood: MEDIUM - Only breaks if google-cloud-storage removes google-crc32c (unlikely but possible)
User Impact: Users calling Service.generate_metadata_from_source_directory() could experience runtime failures in future SDK versions


✅ Approval Criteria

Before merging:

  • Fix the google_crc32c import in _service.py (BLOCKING)
  • Verify make lint passes
  • Verify make test_unit passes
  • Verify CI passes on all platforms
  • (Optional) Add test coverage for generate_metadata_from_source_directory()

Overall Assessment: Good work on the Python 3.14 migration! The dependency updates are comprehensive and well-executed. The one blocking issue is easily fixable and follows the pattern already established in the codebase.
| Branch: feat/python314

@codecov
Copy link

codecov bot commented Dec 6, 2025

Codecov Report

❌ Patch coverage is 72.97297% with 10 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/aignostics/platform/_utils.py 12.50% 7 Missing ⚠️
src/aignostics/application/_download.py 57.14% 3 Missing ⚠️
Files with missing lines Coverage Δ
src/aignostics/__init__.py 85.71% <100.00%> (+2.38%) ⬆️
src/aignostics/application/_service.py 63.80% <100.00%> (-0.06%) ⬇️
src/aignostics/cli.py 90.90% <100.00%> (ø)
src/aignostics/gui/_theme.py 100.00% <100.00%> (ø)
src/aignostics/platform/__init__.py 100.00% <100.00%> (ø)
src/aignostics/platform/_settings.py 85.11% <ø> (-0.68%) ⬇️
src/aignostics/platform/resources/applications.py 69.51% <100.00%> (ø)
src/aignostics/platform/resources/runs.py 61.64% <100.00%> (ø)
src/aignostics/utils/__init__.py 78.26% <ø> (ø)
src/aignostics/utils/_constants.py 78.43% <100.00%> (+0.88%) ⬆️
... and 5 more

... and 15 files with indirect coverage changes

chore(deps): back to pyarrow given fastparquet does not yet support python 3.14, similarly for google_crc32c
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2025

Claude encountered an error —— View job


I'll analyze this and get back to you.

@helmut-hoffer-von-ankershoffen helmut-hoffer-von-ankershoffen merged commit 7182533 into main Dec 6, 2025
19 of 20 checks passed
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 6, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
69.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants