chore: fix the python dependency override#2651
Conversation
yongwww
requested review from
jimmyzho,
kahyunnam,
nvmbreughe and
yzh119
as code owners
Summary of ChangesHello, 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 addresses an issue where Python dependency overrides were not being correctly applied during unit test execution. By strategically reordering and adding a second sourcing of the environment setup script, the changes ensure that custom dependency configurations persist after package installations, leading to a more robust and predictable test environment. 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
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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoved early sourcing of Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner
participant Script as task_run_unit_tests.sh
participant Installer as pip / install_and_verify
participant Setup as setup_test_env.sh
participant Pytest as pytest
CI->>Script: start test job
Script->>Installer: install_and_verify (pip install)
Installer-->>Script: install complete
Script->>Setup: source setup_test_env.sh (reapply overrides)
Script->>Pytest: discover tests (apply norecursedirs), run tests
Pytest-->>Script: test results
Script-->>CI: exit status / results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 Tip Migrating from UI to YAML configuration.Use the |
![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
This pull request correctly fixes an issue where Python dependency overrides were being overwritten by sourcing setup_test_env.sh after the install_and_verify step. However, the initial source call for setup_test_env.sh at the top of the script is now redundant. I've added a comment suggesting its removal to improve script efficiency and maintainability.
scripts/task_run_unit_tests.sh
Outdated
| # Source test environment setup (handles package overrides like TVM-FFI) | ||
| source "${SCRIPT_DIR}/setup_test_env.sh" |
There was a problem hiding this comment.
This source call for setup_test_env.sh is redundant. The script is sourced again within the main function on line 85, which is the correct location to ensure dependency overrides are applied after install_and_verify. Sourcing it here is inefficient as it performs an installation that may be immediately overwritten. This block can be removed to avoid the redundant operation.
There was a problem hiding this comment.
@yongwww makes sense to me can we strive to do it in one place and make it count?
based on the fact you moved it after test_utils.sh so i assume there is no dependency there either, so there's nothing depending on this line anywhere
|
cc: @dierksen |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/task_run_unit_tests.sh (2)
14-16: Consider adding shellcheck disable comment for consistency.Line 12 uses
# shellcheck disable=SC1091for sourcingtest_utils.sh, but this sourcing ofsetup_test_env.shlacks the same directive. For consistency and to suppress potential shellcheck warnings about files not found during static analysis:Proposed fix
-# Source test environment setup (handles package overrides like TVM-FFI) -source "${SCRIPT_DIR}/setup_test_env.sh" +# Source test environment setup (handles package overrides like TVM-FFI) +# shellcheck disable=SC1091 # File exists, checked separately +source "${SCRIPT_DIR}/setup_test_env.sh"🤖 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, Add a shellcheck suppression comment before sourcing setup_test_env.sh to match the existing pattern (like the existing "# shellcheck disable=SC1091" used for test_utils.sh); specifically, insert the same "# shellcheck disable=SC1091" directive immediately above the source "${SCRIPT_DIR}/setup_test_env.sh" line so static analysis won't warn about the sourced file not being found.
84-86: The fix for re-applying dependency overrides is correct.Re-sourcing
setup_test_env.shafterinstall_and_verifyensures pip doesn't overwrite the intended dependency overrides. The comment clearly explains the rationale.Regarding idempotency:
setup_test_env.shsources environment variables idempotently, but runspip install --force-reinstallunconditionally whenTVM_FFI_REFis set. In the current usage context, the script is sourced once, so this works fine. However, if this script were sourced multiple times, the pip install would execute repeatedly. Consider guarding the pip install (e.g., with a flag variable) to make the script truly idempotent for future robustness.🤖 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 84 - 86, The re-source of setup_test_env.sh is correct but its unconditional pip install when TVM_FFI_REF is set makes repeated sourcing non-idempotent; modify setup_test_env.sh to guard the pip install with a sentinel (e.g., a variable like SETUP_TEST_ENV_PIP_DONE) so that when TVM_FFI_REF triggers the pip install (--force-reinstall) it only runs once per shell session; locate the TVM_FFI_REF check and surrounding pip install in setup_test_env.sh and add a conditional that sets and checks the sentinel before running pip, ensuring subsequent sources skip the install.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/task_run_unit_tests.sh`:
- Around line 14-16: Add a shellcheck suppression comment before sourcing
setup_test_env.sh to match the existing pattern (like the existing "# shellcheck
disable=SC1091" used for test_utils.sh); specifically, insert the same "#
shellcheck disable=SC1091" directive immediately above the source
"${SCRIPT_DIR}/setup_test_env.sh" line so static analysis won't warn about the
sourced file not being found.
- Around line 84-86: The re-source of setup_test_env.sh is correct but its
unconditional pip install when TVM_FFI_REF is set makes repeated sourcing
non-idempotent; modify setup_test_env.sh to guard the pip install with a
sentinel (e.g., a variable like SETUP_TEST_ENV_PIP_DONE) so that when
TVM_FFI_REF triggers the pip install (--force-reinstall) it only runs once per
shell session; locate the TVM_FFI_REF check and surrounding pip install in
setup_test_env.sh and add a conditional that sets and checks the sentinel before
running pip, ensuring subsequent sources skip the install.
dierksen
left a comment
dierksen
left a comment
There was a problem hiding this comment.
Could you fix up the duplicate line per the bot suggestion?
yongwww
requested review from
bkryu,
cyx-6,
nv-yunzheq,
saltyminty,
sricketts and
yyihuang
as code owners
3 participants
📌 Description
Fix the python dependency override
🔍 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