-
Notifications
You must be signed in to change notification settings - Fork 0
Add RotorQuant KV cache backend with deferred prefill on Metal #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 8 commits
4557598
53a0907
9c93b6e
ff21598
f40e9d4
aba98c2
8545421
032593b
f587d2b
a43ed82
7ba35fc
f41b97d
3d526b7
3100207
bae6455
c5ec472
13a5f47
04893db
6c4ba6f
d612d14
a50a537
856b30f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -449,16 +449,20 @@ export function SettingsPanel({ open, onClose }: SettingsPanelProps) { | |
| filled | ||
| content={ | ||
| `• Default — No cache quantization. Best baseline quality, highest memory use.\n` + | ||
| `• OptiQ — Rotation-based quantization via mlx-optiq. Best long-context quality.\n` + | ||
| `• RotorQuant Adaptive — IsoQuant 3-bit with deferred prefill, FP16 edge layers. Recommended.\n` + | ||
| `• RotorQuant — IsoQuant 3-bit on all KV layers. Most aggressive compression with deferred prefill.\n` + | ||
| `• OptiQ — Rotation-based quantization via mlx-optiq. Good long-context quality, no GQA support.\n` + | ||
| `• TurboQuant Adaptive — Quantizes middle KV layers, keeps edge layers in FP16. Proven stable.\n` + | ||
| `• TurboQuant — Quantizes all KV layers. Most aggressive compression, higher quality risk.\n` + | ||
| `• TurboQuant — Quantizes all KV layers. Most aggressive non-rotorquant compression.\n` + | ||
| `• MLX Quantized — MLX's built-in cache quantization.\n\n` + | ||
| `Takes effect on next model launch. Incompatible models fall back to Default automatically.` | ||
| `Takes effect on next model launch. OptiQ falls back to Default for unsupported architectures; other backends will error on incompatible models.` | ||
| } | ||
|
Comment on lines
451
to
457
|
||
| /> | ||
| </FieldLabel> | ||
| <Select value={kvBackend} onChange={(e) => setKvBackend(e.target.value)} disabled={!!envOverride}> | ||
| <option value="default">Default (no quantization)</option> | ||
| <option value="rotorquant_adaptive">RotorQuant Adaptive (recommended)</option> | ||
| <option value="rotorquant">RotorQuant</option> | ||
| <option value="optiq">OptiQ (rotation-based)</option> | ||
| <option value="turboquant_adaptive">TurboQuant Adaptive</option> | ||
| <option value="turboquant">TurboQuant</option> | ||
|
|
@@ -467,7 +471,7 @@ export function SettingsPanel({ open, onClose }: SettingsPanelProps) { | |
| {envOverride ? ( | ||
| <HintText>Overridden by SKULK_KV_CACHE_BACKEND environment variable. Remove the env var to configure here.</HintText> | ||
| ) : ( | ||
| <HintText>Changes take effect on the next model launch. Models with incompatible architectures (GQA, non-power-of-two head_dim) will automatically fall back to default.</HintText> | ||
| <HintText>Changes take effect on the next model launch. OptiQ falls back to Default for unsupported architectures; other backends will error on incompatible models.</HintText> | ||
|
||
| )} | ||
|
Comment on lines
474
to
477
|
||
| </Fieldset> | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,24 +181,48 @@ async def _restart_node(self) -> None: | |
|
|
||
| async def _sync_config(self, config_yaml: str) -> None: | ||
| """Write received config YAML to the local config file and | ||
| apply runtime-effective settings (e.g., KV cache backend).""" | ||
| apply runtime-effective settings (e.g., KV cache backend). | ||
|
|
||
| When the user set ``SKULK_KV_CACHE_BACKEND`` at launch, their | ||
| value is the source of truth. The cluster sync must not | ||
| overwrite it — neither in ``os.environ`` nor in the config | ||
| file. | ||
| """ | ||
| config_path = resolve_config_path() | ||
| try: | ||
| import yaml | ||
|
|
||
| raw = yaml.safe_load(config_yaml) | ||
| user_set_kv = bool( | ||
| os.environ.get("_SKULK_KV_BACKEND_USER_SET") | ||
| or os.environ.get("_EXO_KV_BACKEND_USER_SET") | ||
| ) | ||
|
|
||
| # Preserve the user's KV backend in the config file when | ||
| # the env var was set at launch — don't let cluster sync | ||
| # clobber it. | ||
| if user_set_kv and raw and isinstance(raw, dict): | ||
| local_backend = os.environ.get("SKULK_KV_CACHE_BACKEND") | ||
| if local_backend: | ||
|
Comment on lines
+205
to
+206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The sync logic only preserves the local KV backend when Useful? React with 👍 / 👎. |
||
| inference = raw.get("inference") | ||
| if isinstance(inference, dict): | ||
| inference["kv_cache_backend"] = local_backend | ||
|
Comment on lines
+205
to
+209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a launch-time env override is active, Useful? React with 👍 / 👎. |
||
| else: | ||
| raw["inference"] = {"kv_cache_backend": local_backend} | ||
| config_yaml = yaml.safe_dump( | ||
| raw, default_flow_style=False, sort_keys=False | ||
| ) | ||
|
|
||
| config_path.write_text(config_yaml) | ||
| logger.info( | ||
| f"DownloadCoordinator: synced {config_path.name} from cluster ({len(config_yaml)} bytes)" | ||
| ) | ||
| # Apply inference config to env var so next runner spawn picks it up | ||
| import yaml | ||
|
|
||
| raw = yaml.safe_load(config_yaml) | ||
| # Apply inference config to env var so next runner spawn picks it up | ||
| if raw and isinstance(raw, dict): | ||
| inference = raw.get("inference") | ||
| if isinstance(inference, dict) and "kv_cache_backend" in inference: | ||
| # Don't overwrite if user provided the env var at launch | ||
| if not os.environ.get( | ||
| "_SKULK_KV_BACKEND_USER_SET" | ||
| ) and not os.environ.get("_EXO_KV_BACKEND_USER_SET"): | ||
| if not user_set_kv: | ||
| os.environ["SKULK_KV_CACHE_BACKEND"] = str( | ||
| inference["kv_cache_backend"] | ||
| ) | ||
|
|
@@ -210,7 +234,8 @@ async def _sync_config(self, config_yaml: str) -> None: | |
| ) | ||
| else: | ||
| logger.info( | ||
| "DownloadCoordinator: skipping KV backend update (user env var override active)" | ||
| "DownloadCoordinator: keeping user-set KV_CACHE_BACKEND=" | ||
| f"{os.environ.get('SKULK_KV_CACHE_BACKEND', '?')}" | ||
| ) | ||
| # Apply HF token if not user-set | ||
| hf_token = raw.get("hf_token") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| load_exo_config, | ||
| resolve_config_path, | ||
| resolve_node_staging, | ||
| update_config_field, | ||
| ) | ||
| from exo.store.model_store import ModelStore | ||
| from exo.store.model_store_client import ModelStoreClient, ModelStoreDownloader | ||
|
|
@@ -98,12 +99,26 @@ async def create(cls, args: "Args") -> Self: | |
| "1" if _user_set_kv_backend else "" | ||
| ) # legacy compat | ||
|
|
||
| # Apply inference config to env var so runner subprocesses inherit it. | ||
| # Env var takes precedence if user set it at launch. | ||
| if ( | ||
| # Env var is the source of truth for KV backend. When set at launch, | ||
| # write it back to the config file so they stay in sync. When not | ||
| # set, apply the config file value to the env var so runner | ||
| # subprocesses inherit it. | ||
| if _user_set_kv_backend: | ||
| launch_backend = os.environ.get( | ||
| "SKULK_KV_CACHE_BACKEND", | ||
| os.environ.get("EXO_KV_CACHE_BACKEND", ""), | ||
| ) | ||
| if launch_backend: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| if update_config_field("inference", "kv_cache_backend", launch_backend): | ||
| logger.info( | ||
|
Comment on lines
+111
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This persists Useful? React with 👍 / 👎. |
||
| f"Synced launch env KV backend to config: kv_cache_backend={launch_backend}" | ||
| ) | ||
| # Ensure both env vars are in sync | ||
| os.environ["SKULK_KV_CACHE_BACKEND"] = launch_backend | ||
| os.environ["EXO_KV_CACHE_BACKEND"] = launch_backend | ||
|
Comment on lines
+102
to
+118
|
||
| elif ( | ||
| exo_config is not None | ||
| and exo_config.inference is not None | ||
| and not _user_set_kv_backend | ||
| ): | ||
| os.environ["SKULK_KV_CACHE_BACKEND"] = exo_config.inference.kv_cache_backend | ||
| os.environ["EXO_KV_CACHE_BACKEND"] = ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -235,10 +235,38 @@ class InferenceConfig(FrozenModel): | |||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| kv_cache_backend: Literal[ | ||||||||||||||||||||||||||
| "default", "mlx_quantized", "turboquant", "turboquant_adaptive", "optiq" | ||||||||||||||||||||||||||
| "default", | ||||||||||||||||||||||||||
| "mlx_quantized", | ||||||||||||||||||||||||||
| "turboquant", | ||||||||||||||||||||||||||
| "turboquant_adaptive", | ||||||||||||||||||||||||||
| "optiq", | ||||||||||||||||||||||||||
| "rotorquant", | ||||||||||||||||||||||||||
| "rotorquant_adaptive", | ||||||||||||||||||||||||||
| ] = "default" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def update_config_field(section: str, key: str, value: object) -> bool: | ||||||||||||||||||||||||||
| """Update a single field in the config file, preserving all other content. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Reads the raw YAML, patches ``raw[section][key] = value``, and writes | ||||||||||||||||||||||||||
| it back. Returns ``True`` if the file was updated, ``False`` if no | ||||||||||||||||||||||||||
| config file exists. | ||||||||||||||||||||||||||
|
Comment on lines
+249
to
+253
|
||||||||||||||||||||||||||
| """Update a single field in the config file, preserving all other content. | |
| Reads the raw YAML, patches ``raw[section][key] = value``, and writes | |
| it back. Returns ``True`` if the file was updated, ``False`` if no | |
| config file exists. | |
| """Update a single field in the config file, preserving other config data. | |
| Reads the YAML into Python data, patches ``raw[section][key] = value``, | |
| and writes it back. This preserves other parsed configuration values, | |
| but comments and original formatting may be rewritten by PyYAML. | |
| Returns ``True`` if the file was updated, ``False`` if no config file | |
| exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treat config sync writes as best-effort on startup
Writing the synced KV backend uses an unconditional file write with no error handling, and the startup path calls this before the node is fully running. If skulk.yaml exists but is read-only (for example, common config-map style mounts) or the filesystem is temporarily unwritable, this raises and aborts startup even though the env var already provides a valid backend; the sync should fail gracefully instead of taking the node down.
Useful? React with 👍 / 👎.
Copilot
AI
Apr 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This writes the config back with yaml.dump(), which can emit non-safe YAML tags and is generally discouraged compared to yaml.safe_dump() when persisting user-editable config. Prefer safe_dump (and consider setting explicit encoding) to avoid writing Python-specific tags into skulk.yaml/exo.yaml.
| yaml.dump(raw, f, default_flow_style=False, sort_keys=False) | |
| yaml.safe_dump(raw, f, default_flow_style=False, sort_keys=False) |
Copilot
AI
Apr 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_config_field assumes yaml.safe_load() returns a dict and then calls raw.get(...). If the YAML root is not a mapping (or the file is partially corrupted), this will raise and can break startup when SKULK_KV_CACHE_BACKEND is set. Consider guarding with if not isinstance(raw, dict): raw = {} before accessing .get. Also, yaml.dump can emit Python tags for non-primitive values; yaml.safe_dump is safer for config output. The docstring claim about “preserving all other content” is also misleading because load/dump will drop comments/formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bullet links to
docs/kv-cache-backends.md, but that page currently doesn’t mention RotorQuant (it still lists only default/mlx_quantized/turboquant/optiq). Either updatedocs/kv-cache-backends.mdin this PR to include RotorQuant, or change the link to the up-to-date page underwebsite/docs/kv-cache-backends.md/ the published docs URL so readers don’t land on stale information.