decouple qwen3 serving from lib contract - #85
Conversation
📝 WalkthroughWalkthroughThe change defines a lib-owned serving contract and migrates Qwen3 compilation, weight preparation, and runtime kernel argument construction to that contract. Batching tests now obtain contracts and prepared weights through pypto-lib. ChangesContract-driven serving
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Qwen314BExecutor
participant pyptoLibContractRegistry
participant ModelServingContract
participant Qwen314BModelRunner
Qwen314BExecutor->>pyptoLibContractRegistry: find Qwen3 contract
pyptoLibContractRegistry-->>Qwen314BExecutor: ModelServingContract
Qwen314BExecutor->>ModelServingContract: load, validate, bind, and compile stages
ModelServingContract-->>Qwen314BExecutor: compiled kernels and prepared weights
Qwen314BModelRunner->>ModelServingContract: build prefill/decode runtime arguments
ModelServingContract-->>Qwen314BModelRunner: raw kernel ABI tuples
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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 |
bumble0918
changed the title
![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 implements a decoupled architecture between pypto-serving and pypto-lib by refactoring the Qwen3-14B executor and runner to use a contract-based registry instead of hardcoded kernel paths, constants, and layouts. The review feedback identifies critical issues where removing the pypto-lib directory from sys.path in finally blocks will cause ModuleNotFoundError during deferred imports. Additionally, the feedback points out a potential TypeError in both the executor code and the design documentation where contract is redundantly passed as an explicit argument to validate_kernels.
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.
| def _get_pypto_lib_qwen3_contract(model: RuntimeModel) -> object: | ||
| """Select the Qwen3 contract through pypto-lib's registry.""" | ||
| sys.path.insert(0, str(_PYPTO_LIB_DIR)) | ||
| try: | ||
| spec.loader.exec_module(module) | ||
| from contract.registry import find_contract_for_model_config # noqa: PLC0415 | ||
|
|
||
| return find_contract_for_model_config(model.config) | ||
| finally: | ||
| try: | ||
| sys.path.remove(str(_PYPTO_LIB_QWEN14B_DIR)) | ||
| sys.path.remove(str(_PYPTO_LIB_DIR)) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
Removing _PYPTO_LIB_DIR from sys.path in the finally block of _get_pypto_lib_qwen3_contract will cause subsequent calls to contract methods (such as contract.load_kernels() on line 130 or contract.prepare_weights() on line 157) to fail with ModuleNotFoundError if they perform any dynamic or deferred imports from the pypto-lib submodule. Since pypto-lib is a submodule and not installed as a package, _PYPTO_LIB_DIR must remain in sys.path for the duration of the model loading, compilation, and execution process.
def _get_pypto_lib_qwen3_contract(model: RuntimeModel) -> object:
"""Select the Qwen3 contract through pypto-lib's registry."""
if str(_PYPTO_LIB_DIR) not in sys.path:
sys.path.insert(0, str(_PYPTO_LIB_DIR))
from contract.registry import find_contract_for_model_config # noqa: PLC0415
return find_contract_for_model_config(model.config)| def _qwen3_contract(): | ||
| import sys | ||
|
|
||
| sys.path.insert(0, str(PYPTO_LIB_DIR)) | ||
| try: | ||
| from contract.registry import get_contract | ||
|
|
||
| return get_contract("qwen3", "14b") | ||
| finally: | ||
| try: | ||
| sys.path.remove(str(PYPTO_LIB_DIR)) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
Just like in npu_executor.py, removing PYPTO_LIB_DIR from sys.path in the finally block of _qwen3_contract will cause subsequent calls to contract methods (such as prepare_weights on line 576) to fail with ModuleNotFoundError if they perform any dynamic or deferred imports from the pypto-lib submodule.
def _qwen3_contract():
import sys
if str(PYPTO_LIB_DIR) not in sys.path:
sys.path.insert(0, str(PYPTO_LIB_DIR))
from contract.registry import get_contract
return get_contract("qwen3", "14b")| ) | ||
| contract = _get_pypto_lib_qwen3_contract(model) | ||
| loaded_kernels = contract.load_kernels() | ||
| contract.validate_kernels(contract, loaded_kernels, _contract_validation_model(model, contract)) |
There was a problem hiding this comment.
Calling contract.validate_kernels(contract, loaded_kernels, ...) passes contract explicitly as the first positional argument. If validate_kernels is defined as a standard instance method on ModelServingContract (i.e., def validate_kernels(self, loaded_kernels, model)), calling it this way will result in a TypeError because contract is passed twice (once implicitly as self, and once explicitly as the first positional argument).
| contract.validate_kernels(contract, loaded_kernels, _contract_validation_model(model, contract)) | |
| contract.validate_kernels(loaded_kernels, _contract_validation_model(model, contract)) |
|
|
||
| ```python | ||
| loaded_kernels = contract.load_kernels() | ||
| contract.validate_kernels(contract, loaded_kernels, model) |
There was a problem hiding this comment.
In the example code, contract.validate_kernels(contract, loaded_kernels, model) explicitly passes contract as the first positional argument. If validate_kernels is an instance method, calling it this way will result in a TypeError in Python. It should be updated to match the standard instance method invocation.
| contract.validate_kernels(contract, loaded_kernels, model) | |
| contract.validate_kernels(loaded_kernels, model) |
![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
🧹 Nitpick comments (2)
examples/model/qwen3_14b/runner/npu_runner.py (1)
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
contract: objectblocks static attribute access checks.
contractis later accessed asself._compiled.contract.kernels["prefill"].runtime_args_builder(...)(lines 718, 733). Under mypy,objectonly supports operations valid for all objects (isinstance, equality); attribute access like.kernelsis flagged as[attr-defined], unlikeAny. If this repo runs a type checker, this field's usage will fail static checks.🔧 Suggested fix
- contract: object + contract: AnyConsider a
Protocolcapturing the minimalkernels: dict[str, ...]/runtime_args_builderinterface for stronger typing, if feasible.🤖 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 `@examples/model/qwen3_14b/runner/npu_runner.py` at line 82, Replace the `contract: object` annotation in the relevant runner class with a structural type describing the accessed interface: a `kernels` mapping containing entries that expose `runtime_args_builder(...)`. Update the compiled-contract annotation to use this protocol so the accesses in the prefill paths remain type-safe under mypy.tests/test_batching.py (1)
192-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache
_qwen3_contract()to avoid repeatedsys.pathchurn.This helper mutates the global
sys.pathand re-imports on every call; it's invoked from every_compiled_kernels(...)call across the test module plus directly, so the path insert/remove happens far more often than needed for what should be a stable lookup.♻️ Suggested fix
+import functools + + +@functools.lru_cache(maxsize=1) def _qwen3_contract(): import sys sys.path.insert(0, str(PYPTO_LIB_DIR)) try: from contract.registry import get_contract return get_contract("qwen3", "14b") finally: try: sys.path.remove(str(PYPTO_LIB_DIR)) except ValueError: pass🤖 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_batching.py` around lines 192 - 206, Cache the result of _qwen3_contract() so the contract lookup and temporary sys.path insertion/removal occur only once. Apply the cache at the helper boundary, preserving the existing get_contract("qwen3", "14b") behavior and return value for all callers, including direct calls and _compiled_kernels(...).
🤖 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 `@examples/model/qwen3_14b/runner/npu_executor.py`:
- Around line 50-61: Update the wrapper compilation logic to use the stage
spec’s public_name attribute wherever it currently accesses stage.name. Preserve
the existing contract lookup flow and ensure all generated wrapper references
use the supported public_name contract surface.
In `@pypto-lib`:
- Line 1: Update the pypto-lib submodule gitlink from the unreachable commit
6c7f71f69986b2072664a02f573d5df5f8bc4f93 to a commit that is available from the
configured pypto-lib repository, preserving the submodule reference and ensuring
it can be fetched by clean checkouts.
---
Nitpick comments:
In `@examples/model/qwen3_14b/runner/npu_runner.py`:
- Line 82: Replace the `contract: object` annotation in the relevant runner
class with a structural type describing the accessed interface: a `kernels`
mapping containing entries that expose `runtime_args_builder(...)`. Update the
compiled-contract annotation to use this protocol so the accesses in the prefill
paths remain type-safe under mypy.
In `@tests/test_batching.py`:
- Around line 192-206: Cache the result of _qwen3_contract() so the contract
lookup and temporary sys.path insertion/removal occur only once. Apply the cache
at the helper boundary, preserving the existing get_contract("qwen3", "14b")
behavior and return value for all callers, including direct calls and
_compiled_kernels(...).
🪄 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: f7964068-78b5-4022-a4de-b994564cb7c5
📒 Files selected for processing (5)
docs/lib-serving-decoupling-architecture.mdexamples/model/qwen3_14b/runner/npu_executor.pyexamples/model/qwen3_14b/runner/npu_runner.pypypto-libtests/test_batching.py
| def _get_pypto_lib_qwen3_contract(model: RuntimeModel) -> object: | ||
| """Select the Qwen3 contract through pypto-lib's registry.""" | ||
| sys.path.insert(0, str(_PYPTO_LIB_DIR)) | ||
| try: | ||
| spec.loader.exec_module(module) | ||
| from contract.registry import find_contract_for_model_config # noqa: PLC0415 | ||
|
|
||
| return find_contract_for_model_config(model.config) | ||
| finally: | ||
| try: | ||
| sys.path.remove(str(_PYPTO_LIB_QWEN14B_DIR)) | ||
| sys.path.remove(str(_PYPTO_LIB_DIR)) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
echo "== file outline =="
ast-grep outline examples/model/qwen3_14b/runner/npu_executor.py --view expanded || true
echo
echo "== relevant lines in executor =="
nl -ba examples/model/qwen3_14b/runner/npu_executor.py | sed -n '1,340p'
echo
echo "== contract docs references =="
rg -n "public_name|compile_args_builder|runtime_args_builder|host_jit_fn|stage\.name|name field" docs examples/model/qwen3_14b -S || trueRepository: hw-native-sys/pypto-serving
Length of output: 1155
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('examples/model/qwen3_14b/runner/npu_executor.py')
text = p.read_text()
for needle in ['_get_pypto_lib_qwen3_contract', 'load_kernels', 'prepare_weights', '_compile_contract_stage', 'stage.name', 'stage.public_name']:
print(f'== {needle} ==')
idx = text.find(needle)
if idx == -1:
print('not found')
continue
start = max(0, text.rfind('\n', 0, idx-200))
end = min(len(text), text.find('\n', idx+400))
print(text[start:end])
PYRepository: hw-native-sys/pypto-serving
Length of output: 3671
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
docs = Path('docs/lib-serving-decoupling-architecture.md')
if docs.exists():
text = docs.read_text()
for needle in ['public_name', 'compile_args_builder', 'runtime_args_builder', 'host_jit_fn']:
print(f'== {needle} ==')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
else:
print('docs file missing')
PYRepository: hw-native-sys/pypto-serving
Length of output: 1131
🏁 Script executed:
set -euo pipefail
echo "== executor excerpt =="
nl -ba examples/model/qwen3_14b/runner/npu_executor.py | sed -n '40,170p'
echo
echo "== stage/contract excerpt =="
nl -ba examples/model/qwen3_14b/runner/npu_executor.py | sed -n '240,310p'
echo
echo "== docs excerpt =="
nl -ba docs/lib-serving-decoupling-architecture.md | sed -n '210,230p'Repository: hw-native-sys/pypto-serving
Length of output: 228
🏁 Script executed:
set -euo pipefail
echo "== pypto-lib references =="
rg -n "pypto-lib|public_name|compile_args_builder|runtime_args_builder|host_jit_fn|stage\.name|KernelSpec|contract\.kernels|find_contract_for_model_config" . -S
echo
echo "== likely contract/spec definition files =="
fd -a "kernel" .
fd -a "contract" .
fd -a "spec" .
echo
echo "== focused search around KernelSpec / stage fields =="
rg -n "class .*KernelSpec|public_name|name:|host_jit_fn|compile_args_builder|runtime_args_builder" docs examples/model/qwen3_14b . -SRepository: hw-native-sys/pypto-serving
Length of output: 21275
🏁 Script executed:
set -euo pipefail
echo "== sys.path import patterns =="
rg -n "sys\.path\.insert|sys\.path\.remove|importlib\.import_module|from contract\.registry|load_kernels\(|prepare_weights\(|kernel_binder\(" examples/model -S
echo
echo "== deepseek executor import helper =="
python3 - <<'PY'
from pathlib import Path
p = Path('examples/model/deepseek_v4/runner/npu_executor.py')
text = p.read_text().splitlines()
for start in [200, 380]:
end = min(len(text), start + 90)
print(f'--- lines {start+1}-{end} ---')
for i in range(start, end):
print(f'{i+1:4d}: {text[i]}')
PY
echo
echo "== qwen executor surrounding compile path =="
python3 - <<'PY'
from pathlib import Path
p = Path('examples/model/qwen3_14b/runner/npu_executor.py')
text = p.read_text().splitlines()
for start in [118, 244]:
end = min(len(text), start + 70)
print(f'--- lines {start+1}-{end} ---')
for i in range(start, end):
print(f'{i+1:4d}: {text[i]}')
PYRepository: hw-native-sys/pypto-serving
Length of output: 16831
Use stage.public_name here
The stage spec exposes public_name; stage.name is not part of the contract surface, so this will raise when compiling the wrappers.
🤖 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 `@examples/model/qwen3_14b/runner/npu_executor.py` around lines 50 - 61, Update
the wrapper compilation logic to use the stage spec’s public_name attribute
wherever it currently accesses stage.name. Preserve the existing contract lookup
flow and ensure all generated wrapper references use the supported public_name
contract surface.
| @@ -1 +1 @@ | |||
| Subproject commit 3bbd72d00b9b90fe6e70b9077840b3cf6f978f73 | |||
| Subproject commit 6c7f71f69986b2072664a02f573d5df5f8bc4f93 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git -C pypto-lib cat-file -e 6c7f71f69986b2072664a02f573d5df5f8bc4f93^{commit}
git -C pypto-lib grep -nE 'serving contract|contract registry|prepared weights' \
6c7f71f69986b2072664a02f573d5df5f8bc4f93 -- .Repository: hw-native-sys/pypto-serving
Length of output: 341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== submodule config =="
git config -f .gitmodules --get-regexp '^submodule\.' || true
echo
echo "== gitlink =="
git ls-tree HEAD pypto-lib || true
echo
echo "== remote for submodule =="
git -C pypto-lib remote -v || true
echo
echo "== any refs matching the pin prefix =="
git -C pypto-lib show-ref | rg '6c7f71f69986b2072664a02f573d5df5f8bc4f93|pypto-lib' || trueRepository: hw-native-sys/pypto-serving
Length of output: 851
Pinned submodule commit is unreachable
pypto-lib points to 6c7f71f69986b2072664a02f573d5df5f8bc4f93, but that commit isn’t available from https://github.com/hw-native-sys/pypto-lib.git. Update the gitlink to a fetchable commit before merging.
🤖 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 `@pypto-lib` at line 1, Update the pypto-lib submodule gitlink from the
unreachable commit 6c7f71f69986b2072664a02f573d5df5f8bc4f93 to a commit that is
available from the configured pypto-lib repository, preserving the submodule
reference and ensuring it can be fetched by clean checkouts.
1 participant
decouple qwen3 serving from lib contract