Skip to content

Conversation

@kurisu6912
Copy link
Collaborator

@kurisu6912 kurisu6912 commented Nov 4, 2025

This pr remove unsupported type params in @T.prim_func. type params is support since python 3.12.4. We also downgrade the python version in CI to discover these problem more quickly in the future.

Summary by CodeRabbit

  • Chores
    • CI workflow pinned to Python 3.9 (was 3.12) to broaden compatibility.
  • Bug Fixes / Maintenance
    • Simplified annotation handling to avoid unused type-parameter processing.
    • Streamlined data-type construction path to remove a low-level dependency and avoid temporary mutations.

@github-actions
Copy link

github-actions bot commented Nov 4, 2025

👋 Hi! Thank you for contributing to the TileLang project.

Please remember to run pre-commit run --all-files in the root directory of the project to ensure your changes are properly linted and formatted. This will help ensure your contribution passes the format check.

We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

CI Python version changed from 3.12 to 3.9; builder type-hint handling no longer reads func.type_params; dtype construction removed tvm_ffi usage and delegates to the original dtype new for string/mapped inputs.

Changes

Cohort / File(s) Summary
CI Python Version Downgrade
.github/workflows/ci.yml
Updated Setup Python step: python-version changed from 3.12 to 3.9 and job name adjusted accordingly.
Type Hints Adjustment
tilelang/language/v2/builder.py
Removed retrieval of func.__type_params__ and eliminated type_params argument when calling _eval_type(); comment notes type_params are unused (supported since Python 3.12.4).
DType construction cleanup
tilelang/language/v2/dtypes.py
Removed tvm_ffi import and manual construction/attachment of tvm-ffi DataType; introduced backup __orig_dtype_new and delegate to it for string and mapped dtype inputs, removing temporary mutation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to tilelang/language/v2/dtypes.py changes for any subtle behavioral differences when constructing dtypes from strings/mappings.
  • Verify builder.py annotation evaluation still resolves forward refs correctly without type_params.
  • Confirm CI change aligns with project support policy and any test matrix expectations.

Suggested reviewers

  • LeiWang1999

Poem

🐰 I hopped through code both new and old,
Swapped versions gently, tidy and bold.
Type params tucked away with care,
Dtypes cleaned without a scare.
A nibble, a tweak — carrots for the build! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of the PR: removing unsupported type parameters to fix compatibility issues introduced by Python version downgrade.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dcac37a and 11afe4a.

📒 Files selected for processing (1)
  • tilelang/language/v2/dtypes.py (1 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 for Python 3.12 with Metal (on macos-latest)
🔇 Additional comments (1)
tilelang/language/v2/dtypes.py (1)

102-112: LGTM! Clean simplification of dtype construction.

The changes correctly capture the original dtype.__new__ before monkeypatching and delegate to it for both string values and mapped types. This simplifies the code by removing manual tvm_ffi DataType object creation while maintaining the same public API. The delegation pattern is safe and avoids circular dependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)

59-62: Consider adding Python 3.9 to the test matrix for comprehensive compatibility testing.

While downgrading the lint job to Python 3.9 aligns with the removal of Python 3.12.4+ specific features in builder.py, the test matrix (line 101) still only runs on Python 3.12. This creates a gap where runtime compatibility issues with Python 3.9 may not be detected during testing, even though the code is intended to support it.

Consider adding Python 3.9 to the test matrix to ensure comprehensive compatibility:

        python-version:
+         - "3.9"
          - "3.12"

This would provide confidence that the changes actually work on Python 3.9, not just that they pass linting and AST checks.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 778b97d and dcac37a.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml (1 hunks)
  • tilelang/language/v2/builder.py (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T13:29:29.347Z
Learnt from: XuehaiPan
Repo: tile-ai/tilelang PR: 973
File: .github/workflows/ci.yml:13-15
Timestamp: 2025-10-10T13:29:29.347Z
Learning: In .github/workflows/ci.yml for tilelang (GitHub Actions), actions/cachev4 and setup-python’s cache feature require GITHUB_TOKEN with actions: write to save caches; with a permissions block that only sets contents: read, unspecified actions permission becomes none, so caches will restore but not save.

Applied to files:

  • .github/workflows/ci.yml
⏰ 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)
  • GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
  • GitHub Check: Test for Python 3.12 with CUDA-12.8 (on self-hosted-nvidia)
🔇 Additional comments (2)
tilelang/language/v2/builder.py (2)

539-540: LGTM - Correctly removes Python 3.12.4+ dependency.

The comment clearly documents why type_params are not being used, and commenting out the retrieval of __type_params__ (instead of deleting) preserves useful context. This change enables compatibility with Python 3.9 while maintaining the same annotation resolution behavior.


563-564: LGTM - Correctly updates _eval_type call for Python 3.9 compatibility.

The type_params parameter was only added to _eval_type in Python 3.12+. The change correctly removes this parameter since the codebase does not use PEP 695 type parameters. The function will continue to resolve forward references correctly with just the globalns and localns arguments.

@LeiWang1999 LeiWang1999 merged commit 1768cbe into tile-ai:main Nov 4, 2025
5 of 6 checks passed
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