python: strip boilerplate (~140 LOC removed from wrappers) - #12
Conversation
Add inference/c_binder.py exposing bind(family, abi). Each family declares a verb -> (argtypes, restype) ABI dict and the binder opens libsk.dylib (SK_DYLIB env / build/libsk.dylib), resolves sk_<family>_<verb>, and applies signatures. Optional symbols use optional() and are silently skipped when absent. Collapses each wrapper's _load() from ~25-35 lines of repetitive .argtypes/.restype assignments to a single bind() call plus a small ABI dict. Net Python LOC: -41. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move close/__enter__/__exit__/__del__/__repr__ to inference.generation.Model. Subclasses set _destroy_fn (and optional _repr_fields tuple); the base handles handle-nulling, _w_keep clearing, and a uniform repr template. Have Gemma4, DeepSeek, and Mamba2Model inherit Model so all four families share the lifecycle path. Drop the per-family duplicate methods. Net Python LOC across wrappers: -52 (counting the +36 added to generation.py base). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add CtypesConfig mixin in inference/c_binder.py providing to_c(struct_cls) that copies dataclass fields into a Structure, skipping fields the Structure doesn't declare. All four family Config dataclasses now inherit it; per- family _to_c / _to_cstruct bodies collapse to a single call. Drops the unused asdict imports too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both wrappers inherit Model and now use base.generate. Qwen had no own generate; remove the unused prefill/decode_step shims (no external callers). DeepSeek's duplicate generate/prefill/decode_step are deleted; add a base-compatible _forward and keep forward() as a thin int wrapper. DeepSeek's chat() now passes max_new_tokens by keyword (base.generate's arg is keyword-only) and forwards the tokenizer's eos_id. Gemma4 keeps its bf16 last_logits path / sample loop (numerics); Mamba2 has no generate yet and is untouched here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50a6b07ac6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cfg.time_step_min, cfg.time_step_max, cfg.tie_word_embeddings, | ||
| ) | ||
| if lib_path is not None: | ||
| os.environ.setdefault("SK_DYLIB", lib_path) |
There was a problem hiding this comment.
Passing lib_path no longer guarantees that path is used: os.environ.setdefault("SK_DYLIB", lib_path) is a no-op when SK_DYLIB is already set, and bind(...) then resolves/returns the library from environment (and its cache) instead. In processes that preconfigure SK_DYLIB or have already loaded another dylib, Mamba2Model(lib_path=...) can silently bind against the wrong native library, leading to incorrect symbol resolution or ABI mismatches.
Useful? React with 👍 / 👎.
1 participant
Summary
Strip identical boilerplate across the four model Python wrappers (qwen, gemma4, mamba2, deepseek). Goal was lean Python, not abstraction for its own sake — each new infra file is intentionally small.
The four wrappers shrank from 1,349 -> 1,206 LOC (-143). Net across all touched files (including +61 c_binder.py and +35 in inference/generation.py): -47 LOC.
Commit 5 (factory consolidation) was skipped per the spec's hard rule — replacing qwen's 103-line
__init__.pywould have required ~80 LOC of new factory infra for ~80 LOC saved, i.e. no real reduction. The other three families don't have working__init__.pyfactories yet, so there's nothing else to dedup there today.inference/c_binder.py— centralize ctypes bindinginference/c_binder.py(+61),models/qwen/qwen.py,models/deepseek/deepseek.py,models/gemma/gemma4/gemma4.py,models/mamba2/mamba2.pyModelbaseinference/generation.py, all four wrappersCtypesConfigmixin —Config -> ctypes.Structureconversioninference/c_binder.py(already created), all four wrappersgenerate/prefill/decode_stepmodels/qwen/qwen.py,models/deepseek/deepseek.pyWhat stays per-family (deliberately not deduped)
_Configand_Weightsctypes.Structurefield lists — real schema, must match the C ABI.<|turn|>markers, deepseekencode_chat).generate/_sample— bf16 numerics differ from the fp16 path that the baseModel.generateassumes.num_kv_shared_layers/ PLE handling, deepseek's MoE quant + V3 router knobs.__init__.pyfactory and gemma4'sfrom_pretrained— see "Commit 5 skipped" note above.Validation evidence
.metal/ C++ touched; the existingbuild/libsk.dylibis reused.python3 SuperKittens/temp/qwen3_validate/sk_q8_bench.pyshould still return 358 for "Hi!".Test plan
./build.shon lexie -> "Build complete."python3 -c 'import SuperKittens as sk; import SuperKittens.models.qwen; m = sk.load("qwen3-0.6b", quant="q8_0"); print(m.chat("Hi!", max_new_tokens=4))'on lexie -> non-empty reply containing token 358 on first step.🤖 Generated with Claude Code