Skip to content

Conversation

@thomas-chauchefoin-tob
Copy link
Collaborator

This version solves the build error with Python 3.13 (and the associated failing test) while maintaining compatibility with Python 3.10.

This version solves the build error with Python 3.13 (and the associated failing test) while maintaining compatibility with Python 3.10.
Copy link
Member

@dguido dguido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep Analysis: Bump minimum numpy to 2.2.6

Summary

The PR simplifies numpy versioning by replacing Python-version-specific constraints with a single numpy >= 2.2.6 floor, and re-enables a previously skipped test.

Critical Findings

1. Breaking Change for Python 3.10 Users ⚠️

Python Current (torch extras) PR (torch extras)
3.10 numpy >= 1.24.0 → resolves to 2.0.2 numpy >= 2.2.6 → resolves to 2.2.6
3.11-3.13 numpy >= 1.24.0 → resolves to 2.0.2 numpy >= 2.2.6 → resolves to 2.3.5
3.14 numpy >= 2.3.5 → resolves to 2.3.5 numpy >= 2.2.6 → resolves to 2.3.5

The specification change from >= 1.24.0 to >= 2.2.6 is a breaking change. Users who explicitly depend on numpy 1.x behavior can no longer use fickling's torch extras.

2. Lock File Behavior is Correct but Implicit

The lock file shows:

  • numpy 2.2.6 for python_full_version < '3.11' (Python 3.10 only)
  • numpy 2.3.5 for python_full_version >= '3.11' (Python 3.11+)

This happens because NumPy 2.3.x dropped Python 3.10 support. uv correctly resolves the highest satisfying version per Python. However, this implicit split isn't visible in pyproject.toml.

3. Test Skip Removal Lacks Documentation

The PR removes @unittest.skip("FIXME: Failing for python 3.13") from test_zip_properties without documenting:

  • What was the actual failure?
  • Why does numpy 2.2.6 fix it?
  • Was it a numpy bug or a test bug?

4. Comparison with anamorpher #23 Lessons

Lesson from anamorpher #23 Applied here?
Pin numpy to avoid compatibility issues ❌ Uses floor >= instead of pin
Document test fix reasoning ❌ No explanation of why test now passes
Test full Python version matrix ✅ CI passes 3.10-3.14
Warn about dependency interaction fragility ❌ No warning in PR description

Recommendations

  1. Add PR description detail: Document what specific error numpy 2.2.6 fixes

  2. Consider explicit markers: Instead of relying on uv resolution:

    torch = [
        "torch >= 2.1.0",
        "torchvision >= 0.24.1",
        "numpy >= 2.2.6, < 2.3; python_version == '3.10'",
        "numpy >= 2.3.5; python_version >= '3.11'",
    ]
  3. Add comment in test file: Document why test_zip_properties was failing and why it's now fixed

Verdict

The PR is technically correct (CI passes all Python versions), but the implicit breaking change and lack of documentation are concerning. Version matrices for ML dependencies are fragile and benefit from explicit documentation.

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.

3 participants