add qwen3 test ci#49
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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
This pull request updates the pypto-lib subproject commit and adds a new accuracy test suite for the Qwen3 model in tests/test_qwen3_accuracy.py. The review feedback points out critical issues in the test setup, including a potential TypeError during pytest collection if the PYPTO_QWEN3_MODEL_DIR environment variable is unset, a subsequent AttributeError when checking the directory, and a resource leak vulnerability where the executor is not guaranteed to close if the LLMEngine initialization fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| sys.path.insert(0, str(ROOT)) | ||
|
|
||
|
|
||
| MODEL_DIR = Path(os.environ.get("PYPTO_QWEN3_MODEL_DIR")) |
There was a problem hiding this comment.
If the PYPTO_QWEN3_MODEL_DIR environment variable is not set, os.environ.get returns None. Passing None to Path() raises a TypeError at import time. This will crash pytest test collection for the entire test suite, even when running unrelated tests.\n\nWe should safely handle the missing environment variable at import time and fail or skip inside the test function instead.
MODEL_DIR_ENV = os.environ.get("PYPTO_QWEN3_MODEL_DIR")\nMODEL_DIR = Path(MODEL_DIR_ENV) if MODEL_DIR_ENV else None| if not MODEL_DIR.is_dir(): | ||
| pytest.fail(f"Qwen3 model weights not found: {MODEL_DIR}") |
There was a problem hiding this comment.
Since MODEL_DIR can now be None if the environment variable is missing, we should check if it is valid before calling .is_dir() to avoid an AttributeError.
if not MODEL_DIR or not MODEL_DIR.is_dir():\n pytest.fail(f"Qwen3 model weights not found: {MODEL_DIR}")| engine = LLMEngine(kv_cache_manager=kv_cache_manager, executor=executor) | ||
|
|
||
| try: |
There was a problem hiding this comment.
If LLMEngine initialization fails, the executor (which may have already allocated NPU resources) will not be closed because the try...finally block starts after its initialization. Move the try block up to immediately follow the creation of executor to ensure resources are always cleaned up.
try:\n engine = LLMEngine(kv_cache_manager=kv_cache_manager, executor=executor)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
29-84: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the workflow to least-privilege permissions.
This workflow still relies on the default
GITHUB_TOKENpermissions. Since this job only checks out code, installs dependencies, builds wheels, and runs tests, it should declare an explicit read-onlypermissions:block instead of inheriting the broader default.Suggested fix
jobs: unit-tests: + permissions: + contents: read runs-on: ubuntu-latest timeout-minutes: 30🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 29 - 84, The unit-tests job is inheriting the default GITHUB_TOKEN scope instead of using least-privilege access. Update the workflow to add an explicit read-only permissions block for the job or workflow, and keep it limited to the access needed by actions/checkout and the test steps. Use the existing unit-tests job in ci.yml as the place to apply the change.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 80-84: The new Qwen3 accuracy guard step is hard-wired to fail on
the current GitHub-hosted runner because Qwen3 test setup assumes a local model
directory that does not exist there. Update the workflow job that runs “Run
Qwen3 accuracy guard” to either use a runner with the Qwen3 model mounted or add
a pre-check before invoking tests/test_qwen3_accuracy.py so the step is skipped
when the model directory is unavailable; reference the PYPTO_QWEN3_MODEL_DIR
environment setting and the test’s failure path in tests/test_qwen3_accuracy.py
when making the change.
In `@tests/test_qwen3_accuracy.py`:
- Line 26: Avoid evaluating PYPTO_QWEN3_MODEL_DIR at import time in
test_qwen3_accuracy.py, since MODEL_DIR = Path(os.environ.get(...)) can crash
pytest collection when the variable is unset. Move the env var lookup into the
test setup or test body used by the Qwen3 accuracy test, and if the value is
missing, call pytest.skip(...) before constructing Path or using MODEL_DIR; keep
the change localized around MODEL_DIR and the test that depends on it.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 29-84: The unit-tests job is inheriting the default GITHUB_TOKEN
scope instead of using least-privilege access. Update the workflow to add an
explicit read-only permissions block for the job or workflow, and keep it
limited to the access needed by actions/checkout and the test steps. Use the
existing unit-tests job in ci.yml as the place to apply the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 412e9eae-619f-4039-81ee-a655e01fa4e8
📒 Files selected for processing (3)
.github/workflows/ci.ymlpypto-libtests/test_qwen3_accuracy.py
| - name: Run Qwen3 accuracy guard | ||
| env: | ||
| PYTHONPATH: ${{ github.workspace }} | ||
| PYPTO_QWEN3_MODEL_DIR: /data/100955553/model/Qwen3-14B | ||
| run: python -m pytest tests/test_qwen3_accuracy.py -q -s No newline at end of file |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
This path will not exist on ubuntu-latest, so the new job step is effectively hard-wired to fail.
Lines 80-84 point PYPTO_QWEN3_MODEL_DIR at /data/100955553/model/Qwen3-14B, but this job runs on GitHub-hosted ubuntu-latest. Cross-file, tests/test_qwen3_accuracy.py Lines 37-38 call pytest.fail(...) when that directory is missing, so the new CI lane becomes permanently red unless you switch to a runner that actually has the model mounted or gate the step behind an availability check.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 1-84: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[warning] 29-84: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 80 - 84, The new Qwen3 accuracy guard
step is hard-wired to fail on the current GitHub-hosted runner because Qwen3
test setup assumes a local model directory that does not exist there. Update the
workflow job that runs “Run Qwen3 accuracy guard” to either use a runner with
the Qwen3 model mounted or add a pre-check before invoking
tests/test_qwen3_accuracy.py so the step is skipped when the model directory is
unavailable; reference the PYPTO_QWEN3_MODEL_DIR environment setting and the
test’s failure path in tests/test_qwen3_accuracy.py when making the change.
| sys.path.insert(0, str(ROOT)) | ||
|
|
||
|
|
||
| MODEL_DIR = Path(os.environ.get("PYPTO_QWEN3_MODEL_DIR")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid crashing test collection when PYPTO_QWEN3_MODEL_DIR is unset.
Line 26 evaluates Path(os.environ.get(...)) at import time, so an unset env var raises TypeError before pytest can collect the module. Resolve the env var inside the test and pytest.skip(...) when it is absent; otherwise local/full-suite runs without the model configured will error out immediately.
Suggested fix
-MODEL_DIR = Path(os.environ.get("PYPTO_QWEN3_MODEL_DIR"))
+MODEL_DIR_ENV = os.environ.get("PYPTO_QWEN3_MODEL_DIR")
MODEL_ID = "qwen3-14b-accuracy"
PLATFORM = os.environ.get("PYPTO_QWEN3_PLATFORM", "a2a3")
DEVICE_ID = int(os.environ.get("PYPTO_QWEN3_DEVICE", "0"))
PROMPT = "The capital of France is"
MAX_NEW_TOKENS = 8
@@
def test_qwen3_output_matches_expected_tokens():
- if not MODEL_DIR.is_dir():
- pytest.fail(f"Qwen3 model weights not found: {MODEL_DIR}")
+ if not MODEL_DIR_ENV:
+ pytest.skip("PYPTO_QWEN3_MODEL_DIR is not set")
+ model_dir = Path(MODEL_DIR_ENV)
+ if not model_dir.is_dir():
+ pytest.fail(f"Qwen3 model weights not found: {model_dir}")
@@
engine.init_model(
model_id=MODEL_ID,
- model_dir=str(MODEL_DIR),
+ model_dir=str(model_dir),📝 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.
| MODEL_DIR = Path(os.environ.get("PYPTO_QWEN3_MODEL_DIR")) | |
| MODEL_DIR_ENV = os.environ.get("PYPTO_QWEN3_MODEL_DIR") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_qwen3_accuracy.py` at line 26, Avoid evaluating
PYPTO_QWEN3_MODEL_DIR at import time in test_qwen3_accuracy.py, since MODEL_DIR
= Path(os.environ.get(...)) can crash pytest collection when the variable is
unset. Move the env var lookup into the test setup or test body used by the
Qwen3 accuracy test, and if the value is missing, call pytest.skip(...) before
constructing Path or using MODEL_DIR; keep the change localized around MODEL_DIR
and the test that depends on it.
superxf
force-pushed
the
qwen_ci
branch
5 times, most recently
from
310110c to
799d7be
Compare
2 participants
add qwen3 accuracy ci