[DRAFT][DO NOT MERGE] Test tvm-ffi 0.1.10rc#2599
[DRAFT][DO NOT MERGE] Test tvm-ffi 0.1.10rc#2599yongwww wants to merge 4 commits intoflashinfer-ai:mainfrom
Conversation
Summary of ChangesHello @yongwww, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the project's continuous integration configuration to pin the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCI now pins Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request updates the TVM_FFI_REF version in ci/setup_python.env to v0.1.9-rc0. This change is straightforward and directly addresses the stated objective of upgrading tvm-ffi. The change is minimal and appears correct for its intended purpose.
yongwww
changed the title
|
/bot run |
|
[FAILED] Pipeline #44462250: 14/20 passed |
|
/bot run |
|
/bot stop |
|
The GitLab CI pipeline #44479402 has been cancelled. |
|
/bot run |
|
/bot stop |
|
The GitLab CI pipeline #44485328 has been cancelled. |
|
/bot run |
|
/bot run |
|
[FAILED] Pipeline #44488356: 14/20 passed |
|
/bot run |
|
@flashinfer-bot rerun failed |
|
[FAILED] Pipeline #44569871: 14/20 passed |
|
@flashinfer-bot run failed |
|
/bot run |
|
[FAILED] Pipeline #44644054: 14/20 passed |
yongwww
requested review from
aleozlx,
bkryu,
cyx-6,
jimmyzho,
kahyunnam,
nv-yunzheq,
saltyminty and
yyihuang
as code owners
|
/bot run |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/task_jit_run_tests_part1.sh`:
- Around line 16-20: The current python import line uses "|| true", which
suppresses import errors even when an override is active; change the behavior so
import failures are allowed to fail fast when TVM_FFI_REF is set: detect the
TVM_FFI_REF env var and run the python -c "import tvm_ffi; ..." without "||
true" when TVM_FFI_REF is non-empty, otherwise keep the existing tolerant
behavior (retain "|| true") when no override is active; update the python import
invocation (the line containing python -c "import tvm_ffi; print(...)") and
remove or conditionally apply the "|| true" accordingly (apply same fix to the
other occurrences referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2681cded-0e94-4b79-ac15-97f90183935e
📒 Files selected for processing (4)
ci/setup_python.envscripts/task_jit_run_tests_part1.shscripts/task_run_unit_tests.shscripts/task_test_jit_cache_package_build_import.sh
| # Print TVM-FFI version just before running tests | ||
| echo "Checking TVM-FFI version before tests..." | ||
| python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true | ||
| echo "" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining non-fatal TVM-FFI diagnostics in CI scripts
rg -nP --type=sh 'import tvm_ffi;.*\|\|\s*true'Repository: flashinfer-ai/flashinfer
Length of output: 595
🏁 Script executed:
cat -n scripts/task_jit_run_tests_part1.sh | head -50Repository: flashinfer-ai/flashinfer
Length of output: 1102
🏁 Script executed:
rg -nP 'TVM_FFI_REF' --type=shRepository: flashinfer-ai/flashinfer
Length of output: 333
🏁 Script executed:
cat -n scripts/setup_test_env.sh | head -100Repository: flashinfer-ai/flashinfer
Length of output: 1192
🏁 Script executed:
cat -n scripts/task_test_jit_cache_package_build_import.sh | sed -n '70,125p'Repository: flashinfer-ai/flashinfer
Length of output: 2084
🏁 Script executed:
cat -n scripts/task_run_unit_tests.sh | sed -n '85,100p'Repository: flashinfer-ai/flashinfer
Length of output: 693
Fail the TVM-FFI version check when dependency override is active.
The || true on line 18 suppresses import errors that should block the test when TVM_FFI_REF is set. Testing a custom dependency ref should fail fast if the override is broken, not silently continue.
Suggested fix
# Print TVM-FFI version just before running tests
echo "Checking TVM-FFI version before tests..."
-python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true
+if [ -n "${TVM_FFI_REF:-}" ]; then
+ python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')"
+else
+ python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true
+fi
echo ""Note: The same issue exists at scripts/task_test_jit_cache_package_build_import.sh:77,118 and scripts/task_run_unit_tests.sh:92.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Print TVM-FFI version just before running tests | |
| echo "Checking TVM-FFI version before tests..." | |
| python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true | |
| echo "" | |
| # Print TVM-FFI version just before running tests | |
| echo "Checking TVM-FFI version before tests..." | |
| if [ -n "${TVM_FFI_REF:-}" ]; then | |
| python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" | |
| else | |
| python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true | |
| fi | |
| echo "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/task_jit_run_tests_part1.sh` around lines 16 - 20, The current python
import line uses "|| true", which suppresses import errors even when an override
is active; change the behavior so import failures are allowed to fail fast when
TVM_FFI_REF is set: detect the TVM_FFI_REF env var and run the python -c "import
tvm_ffi; ..." without "|| true" when TVM_FFI_REF is non-empty, otherwise keep
the existing tolerant behavior (retain "|| true") when no override is active;
update the python import invocation (the line containing python -c "import
tvm_ffi; print(...)") and remove or conditionally apply the "|| true"
accordingly (apply same fix to the other occurrences referenced).
|
[FAILED] Pipeline #46802453: 13/20 passed |
|
/bot run |
|
[CANCELING] Pipeline #47560590: canceled |
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/task_run_unit_tests.sh (1)
14-16: Remove the early sourcing of setup_test_env.sh to avoid redundant pip operations.The early source at lines 14–15 executes before any code that depends on its package overrides. All subsequent functions (through line 70) are definitions only, and the main execution block doesn't reach setup_test_env.sh again until line 85. This means when
TVM_FFI_REFis set, pip install/uninstall operations run twice in a single job, adding churn and complicating debugging.Proposed simplification
-# Source test environment setup (handles package overrides like TVM-FFI) -source "${SCRIPT_DIR}/setup_test_env.sh" +# Defer package override application to the post-install source at Line 85 +# to avoid duplicate pip operations in the same run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/task_run_unit_tests.sh` around lines 14 - 16, Remove the early sourcing of setup_test_env.sh at the top of task_run_unit_tests.sh (the `source "${SCRIPT_DIR}/setup_test_env.sh"` call) to prevent duplicate pip install/uninstall when TVM_FFI_REF is set; instead, source setup_test_env.sh only once inside the main execution block immediately before any code that depends on the environment overrides (where the script currently performs runtime actions), leaving all function definitions (the earlier block) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/task_test_jit_cache_package_build_import.sh`:
- Around line 75-79: Update the echo message that precedes the TVM-FFI import to
accurately reflect when it runs: change the string "Checking TVM-FFI version
before verification tests..." to something like "Checking TVM-FFI version before
package installation..." so the log matches the execution point just before the
python -c "import tvm_ffi; ..." check; adjust only the echo text near the python
invocation to avoid confusing CI logs.
---
Nitpick comments:
In `@scripts/task_run_unit_tests.sh`:
- Around line 14-16: Remove the early sourcing of setup_test_env.sh at the top
of task_run_unit_tests.sh (the `source "${SCRIPT_DIR}/setup_test_env.sh"` call)
to prevent duplicate pip install/uninstall when TVM_FFI_REF is set; instead,
source setup_test_env.sh only once inside the main execution block immediately
before any code that depends on the environment overrides (where the script
currently performs runtime actions), leaving all function definitions (the
earlier block) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a21cc0ba-8d8a-457c-85c6-e6ca55b7c5e8
📒 Files selected for processing (4)
ci/setup_python.envscripts/task_jit_run_tests_part1.shscripts/task_run_unit_tests.shscripts/task_test_jit_cache_package_build_import.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/task_jit_run_tests_part1.sh
- ci/setup_python.env
| echo "" | ||
| echo "Checking TVM-FFI version before verification tests..." | ||
| python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true | ||
| echo "" | ||
|
|
There was a problem hiding this comment.
Fix the first TVM-FFI log label to match execution point.
On Line 76, the message says “before verification tests,” but this block runs before package installation. That can mislead CI debugging.
Proposed wording fix
-echo "Checking TVM-FFI version before verification tests..."
+echo "Checking TVM-FFI version before package installation..."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "" | |
| echo "Checking TVM-FFI version before verification tests..." | |
| python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true | |
| echo "" | |
| echo "" | |
| echo "Checking TVM-FFI version before package installation..." | |
| python -c "import tvm_ffi; print(f'TVM-FFI version: {tvm_ffi.__version__}')" || true | |
| echo "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/task_test_jit_cache_package_build_import.sh` around lines 75 - 79,
Update the echo message that precedes the TVM-FFI import to accurately reflect
when it runs: change the string "Checking TVM-FFI version before verification
tests..." to something like "Checking TVM-FFI version before package
installation..." so the log matches the execution point just before the python
-c "import tvm_ffi; ..." check; adjust only the echo text near the python
invocation to avoid confusing CI logs.
|
[FAILED] Pipeline #47602277: 7/20 passed |
|
/bot run |
|
[FAILED] Pipeline #47660310: 9/20 passed |
2 participants
📌 Description
🔍 Related Issues
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Reviewer Notes
Summary by CodeRabbit