-
Notifications
You must be signed in to change notification settings - Fork 401
[tinker] Keep the multi-tenant LoRA runtime warm on last unload #2001
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
Open
avigyabb
wants to merge
1
commit into
NovaSky-AI:main
Choose a base branch
from
avigyabb:tinker-warm-runtime-last-unload
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+275
−15
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -49,10 +49,16 @@ | |||||||||||||||||
| class SkyRLTrainBackendOverrides(BaseModel, extra="allow"): | ||||||||||||||||||
| """Configuration overrides for the SkyRL-Train backend. | ||||||||||||||||||
|
|
||||||||||||||||||
| All keys are applied as overrides to the default SkyRL-Train config. | ||||||||||||||||||
| Declared fields configure the backend itself; all extra keys are applied | ||||||||||||||||||
| as overrides to the default SkyRL-Train config. | ||||||||||||||||||
| """ | ||||||||||||||||||
|
|
||||||||||||||||||
| pass | ||||||||||||||||||
| keep_runtime_warm_on_last_unload: bool = False | ||||||||||||||||||
| """Keep the shared runtime (Ray, training workers, inference engines, base | ||||||||||||||||||
| model) alive when the last LoRA model is unloaded, so the next compatible | ||||||||||||||||||
| ``create_model`` registers a fresh adapter against it instead of | ||||||||||||||||||
| rebuilding. Only applies to LoRA policies; full-parameter fine-tuning | ||||||||||||||||||
| always tears down on unload.""" | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| class FSDPBackendOverrides(SkyRLTrainBackendOverrides): | ||||||||||||||||||
|
|
@@ -131,6 +137,10 @@ def __init__(self, base_model: str, config: SkyRLTrainBackendOverrides): | |||||||||||||||||
| self._tokenizer: AutoTokenizer = get_tokenizer(self.base_model) | ||||||||||||||||||
| self._inference_engine_client = None | ||||||||||||||||||
| self._inference_engines_initialized = False | ||||||||||||||||||
| # Tenants whose LoRA adapters are currently registered on the | ||||||||||||||||||
| # inference engines (populated by save_sampler_checkpoint, drained by | ||||||||||||||||||
| # delete_model so vLLM stops serving deleted tenants). | ||||||||||||||||||
| self._inference_adapter_ids: set[str] = set() | ||||||||||||||||||
| self._renderer = None | ||||||||||||||||||
| # CPU-only render server for multi-modal preprocessing; started | ||||||||||||||||||
| # lazily on the first image-bearing training batch. | ||||||||||||||||||
|
|
@@ -442,12 +452,13 @@ def create_model(self, model_id: str, lora_config: types.LoraConfig, model_role: | |||||||||||||||||
| raise ValueError(f"Model '{model_id}' already exists") | ||||||||||||||||||
|
|
||||||||||||||||||
| is_lora = lora_config is not None and lora_config.rank > 0 | ||||||||||||||||||
| is_first_policy = "policy" not in self._model_ids_to_role.values() | ||||||||||||||||||
|
|
||||||||||||||||||
| # Multi-LoRA path: allow additional policy adapters when LoRA is active | ||||||||||||||||||
| # and the first model has already been built. FFT (rank=0) keeps the | ||||||||||||||||||
| # original single-tenant gate. | ||||||||||||||||||
| if model_role == "policy" and not is_first_policy: | ||||||||||||||||||
| # Multi-LoRA path: register additional policy adapters against the | ||||||||||||||||||
| # already-built shared runtime. Gate on the runtime being alive rather | ||||||||||||||||||
| # than on a policy model being registered: with | ||||||||||||||||||
| # keep_runtime_warm_on_last_unload the runtime outlives the last | ||||||||||||||||||
| # registered model. FFT (rank=0) keeps the original single-tenant gate. | ||||||||||||||||||
| if model_role == "policy" and self._dispatch is not None: | ||||||||||||||||||
| if not is_lora: | ||||||||||||||||||
| raise ValueError( | ||||||||||||||||||
| "SkyRLTrainBackend already has a 'policy' model; multi-tenant " | ||||||||||||||||||
|
|
@@ -535,19 +546,37 @@ def _create_colocate_pg(self): | |||||||||||||||||
|
|
||||||||||||||||||
| return ResolvedPlacementGroup(pg) | ||||||||||||||||||
|
|
||||||||||||||||||
| def _unload_inference_adapter(self, model_id: str) -> None: | ||||||||||||||||||
| """Drop a deleted tenant's LoRA adapter from the inference engines. | ||||||||||||||||||
|
|
||||||||||||||||||
| Only adapters actually registered on vLLM (via save_sampler_checkpoint) | ||||||||||||||||||
| are unloaded. Best-effort: vLLM may have LRU-evicted the adapter | ||||||||||||||||||
| already, and an inference-side failure must not block the tenant's | ||||||||||||||||||
| removal from the training runtime. | ||||||||||||||||||
| """ | ||||||||||||||||||
| if model_id not in self._inference_adapter_ids: | ||||||||||||||||||
| return | ||||||||||||||||||
| try: | ||||||||||||||||||
| asyncio.run(self._inference_engine_client.unload_lora_adapter(model_id)) | ||||||||||||||||||
|
Comment on lines
+557
to
+560
Contributor
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. To prevent potential
Suggested change
|
||||||||||||||||||
| except Exception as e: | ||||||||||||||||||
| logger.warning(f"Failed to unload LoRA adapter '{model_id}' from inference engines: {e}") | ||||||||||||||||||
| self._inference_adapter_ids.discard(model_id) | ||||||||||||||||||
|
|
||||||||||||||||||
| def delete_model(self, model_id: str) -> None: | ||||||||||||||||||
| role = self._get_role(model_id) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Multi-LoRA: if more than one model is currently registered, drop just | ||||||||||||||||||
| # this adapter slot rather than tearing down the shared Ray runtime. | ||||||||||||||||||
| # The live GPU state may still mirror this adapter; it'll be | ||||||||||||||||||
| # Multi-LoRA: if more than one model is currently registered — or the | ||||||||||||||||||
| # last one is unloading with keep_runtime_warm_on_last_unload set — | ||||||||||||||||||
| # drop just this adapter slot rather than tearing down the shared Ray | ||||||||||||||||||
| # runtime. The live GPU state may still mirror this adapter; it'll be | ||||||||||||||||||
| # overwritten on the next swap_to (no eager swap-away here). | ||||||||||||||||||
| if len(self._model_ids_to_role) > 1: | ||||||||||||||||||
| if len(self._model_ids_to_role) > 1 or self.config.keep_runtime_warm_on_last_unload: | ||||||||||||||||||
| if role == "policy" and self._base_lora_signature is not None: | ||||||||||||||||||
| self._unload_inference_adapter(model_id) | ||||||||||||||||||
| self._dispatch.delete_adapter("policy", model_id) | ||||||||||||||||||
| del self._model_ids_to_role[model_id] | ||||||||||||||||||
| self._model_metadata.pop(model_id, None) | ||||||||||||||||||
| logger.info(f"Removed LoRA adapter '{model_id}'") | ||||||||||||||||||
| logger.info(f"Removed LoRA adapter '{model_id}'; shared runtime stays up") | ||||||||||||||||||
| return | ||||||||||||||||||
| # Fall through to teardown for non-LoRA roles or unexpected mixes. | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -570,6 +599,7 @@ def delete_model(self, model_id: str) -> None: | |||||||||||||||||
| self._dispatch = None | ||||||||||||||||||
| self._inference_engine_client = None | ||||||||||||||||||
| self._inference_engines_initialized = False | ||||||||||||||||||
| self._inference_adapter_ids = set() | ||||||||||||||||||
| self._renderer = None | ||||||||||||||||||
| self._colocate_pg = None | ||||||||||||||||||
| self._base_lora_signature = None | ||||||||||||||||||
|
|
@@ -1274,6 +1304,10 @@ def save_sampler_checkpoint(self, output_path, model_id: str, persist: bool = Tr | |||||||||||||||||
| # name. None for the FFT / single-tenant path uses legacy behavior. | ||||||||||||||||||
| sync_id = model_id if self._base_lora_signature is not None else None | ||||||||||||||||||
| asyncio.run(self._dispatch.save_weights_for_sampler(model_id=sync_id)) | ||||||||||||||||||
| if sync_id is not None: | ||||||||||||||||||
| # The sync registered this tenant's adapter on vLLM; remember it | ||||||||||||||||||
| # so delete_model can unload it. | ||||||||||||||||||
| self._inference_adapter_ids.add(model_id) | ||||||||||||||||||
| logger.info(f"Synced weights for {model_id} to inference engines via NCCL") | ||||||||||||||||||
|
|
||||||||||||||||||
| if persist: | ||||||||||||||||||
|
|
||||||||||||||||||
70 changes: 70 additions & 0 deletions
70
tests/backends/skyrl_train/workers/test_adapter_store_lifecycle.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| """CPU tests for AdapterStore's register/delete state machine. | ||
|
|
||
| Pins the stale-live invariant: after the *current* adapter is deleted, the | ||
| live GPU state still mirrors the deleted tenant, so a subsequently created | ||
| adapter must be seeded from the pristine slot (and only become live via | ||
| swap_to) instead of silently inheriting the deleted tenant's weights. | ||
|
|
||
| Tensor plumbing (`_allocate_empty_slot`, `_copy_slot`) is stubbed out — these | ||
| tests exercise only the slot bookkeeping, which is what the multi-tenant | ||
| warm-runtime lifecycle (delete last tenant, create a new one) relies on. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| try: | ||
| from skyrl.backends.skyrl_train.workers.megatron.adapter_store import AdapterStore | ||
| except Exception as e: # noqa: BLE001 — megatron/TE also raise RuntimeError without CUDA libs | ||
| pytest.skip(f"megatron adapter store unavailable: {e}", allow_module_level=True) | ||
|
|
||
| SIGNATURE = object() | ||
|
|
||
|
|
||
| def _store() -> tuple[AdapterStore, list[tuple[object, object]]]: | ||
| """AdapterStore with tensor plumbing stubbed; returns (store, copy_log).""" | ||
| store = AdapterStore() | ||
| store._signature = SIGNATURE | ||
| store._pristine = object() | ||
| copies: list[tuple[object, object]] = [] | ||
| store._allocate_empty_slot = lambda model_chunks, optimizer: object() | ||
| store._copy_slot = lambda src, dst: copies.append((src, dst)) | ||
| return store, copies | ||
|
|
||
|
|
||
| def test_first_create_treats_live_as_authoritative(): | ||
| store, copies = _store() | ||
|
|
||
| store.create("model-a", model_chunks=[], optimizer=None, signature=SIGNATURE) | ||
|
|
||
| assert store.current_id == "model-a" | ||
| assert copies == [] | ||
|
|
||
|
|
||
| def test_create_after_deleting_current_seeds_from_pristine(): | ||
| store, copies = _store() | ||
| store.create("model-a", model_chunks=[], optimizer=None, signature=SIGNATURE) | ||
| store.delete("model-a") | ||
|
|
||
| store.create("model-b", model_chunks=[], optimizer=None, signature=SIGNATURE) | ||
|
|
||
| # Live state still mirrors deleted model-a: model-b's slot must be a | ||
| # pristine copy, and it must not be considered live until swap_to runs. | ||
| assert copies == [(store._pristine, store._slots["model-b"])] | ||
| assert store.current_id is None | ||
|
|
||
|
|
||
| def test_delete_of_non_current_adapter_keeps_live_authoritative(): | ||
| store, copies = _store() | ||
| store.create("model-a", model_chunks=[], optimizer=None, signature=SIGNATURE) | ||
| store.create("model-b", model_chunks=[], optimizer=None, signature=SIGNATURE) | ||
| copies.clear() | ||
|
|
||
| store.delete("model-b") | ||
| store.create("model-c", model_chunks=[], optimizer=None, signature=SIGNATURE) | ||
|
|
||
| # model-a is still current and live; model-c seeds from pristine as any | ||
| # subsequent registration does. | ||
| assert store.current_id == "model-a" | ||
| assert copies == [(store._pristine, store._slots["model-c"])] |
143 changes: 143 additions & 0 deletions
143
tests/tinker/skyrl_train/test_warm_runtime_lifecycle.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| """Unit tests for the multi-tenant warm-runtime lifecycle | ||
| (``keep_runtime_warm_on_last_unload``). | ||
|
|
||
| Covers issue #1654 items 1 and 3: the last LoRA unload can keep the shared | ||
| Ray runtime (workers, inference engines, base model) alive instead of calling | ||
| ``ray.shutdown()``, full-parameter fine-tuning keeps the teardown behavior, | ||
| and the inference proxy URL published at engine bring-up is never cleared or | ||
| re-published in the warm lifecycle (a single upsert for the server lifetime). | ||
|
|
||
| CPU-only: backend state is constructed directly (no Ray, no GPU). Run: | ||
| uv run --isolated --extra tinker --extra fsdp --with pytest \\ | ||
| pytest tests/tinker/skyrl_train/test_warm_runtime_lifecycle.py | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import AsyncMock, Mock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| skyrl_train_backend = pytest.importorskip("skyrl.backends.skyrl_train_backend") | ||
|
|
||
| from skyrl.backends.skyrl_train_backend import ( # noqa: E402 | ||
| MegatronBackendOverrides, | ||
| SkyRLTrainBackend, | ||
| ) | ||
| from skyrl.tinker import types # noqa: E402 | ||
|
|
||
| LORA_CONFIG = types.LoraConfig(rank=8, alpha=16, seed=0) | ||
|
|
||
|
|
||
| def _backend( | ||
| keep_runtime_warm: bool, | ||
| model_ids: tuple[str, ...] = ("model-a",), | ||
| lora: bool = True, | ||
| ) -> SkyRLTrainBackend: | ||
| """Build a backend in the post-create_model state without running __init__.""" | ||
| backend = object.__new__(SkyRLTrainBackend) | ||
| backend.config = MegatronBackendOverrides(keep_runtime_warm_on_last_unload=keep_runtime_warm) | ||
| backend._model_ids_to_role = {model_id: "policy" for model_id in model_ids} | ||
| backend._model_metadata = { | ||
| model_id: types.ModelMetadata(adapter_index=0, lora_config=LORA_CONFIG) for model_id in model_ids | ||
| } | ||
| backend._cfg = Mock() | ||
| backend._dispatch = Mock() | ||
| backend._colocate_pg = None | ||
| backend._inference_engine_client = Mock() | ||
| backend._inference_engine_client.unload_lora_adapter = AsyncMock() | ||
| backend._inference_engines_initialized = True | ||
| backend._inference_adapter_ids = set() | ||
| backend._renderer = None | ||
| backend._render_server = None | ||
| backend._base_lora_signature = (LORA_CONFIG.rank, int(LORA_CONFIG.alpha)) if lora else None | ||
| backend._server_groups = [] | ||
| backend._inference_router = None | ||
| backend._inference_state_publisher = Mock() | ||
| return backend | ||
|
|
||
|
|
||
| def test_last_lora_unload_keeps_runtime_warm_when_enabled(): | ||
| backend = _backend(keep_runtime_warm=True) | ||
| dispatch = backend._dispatch | ||
|
|
||
| with patch("skyrl.backends.skyrl_train_backend.ray.shutdown") as shutdown: | ||
| backend.delete_model("model-a") | ||
|
|
||
| shutdown.assert_not_called() | ||
| dispatch.delete_adapter.assert_called_once_with("policy", "model-a") | ||
| assert backend._dispatch is dispatch | ||
| assert backend._model_ids_to_role == {} | ||
| assert backend._base_lora_signature == (8, 16) | ||
| assert backend._inference_engines_initialized | ||
| # Item 3: the proxy URL published at engine bring-up stays valid — the | ||
| # warm path never clears (None) or re-publishes it. | ||
| backend._inference_state_publisher.assert_not_called() | ||
|
|
||
|
|
||
| def test_last_lora_unload_tears_down_by_default(): | ||
| backend = _backend(keep_runtime_warm=False) | ||
|
|
||
| with patch("skyrl.backends.skyrl_train_backend.ray.shutdown") as shutdown: | ||
| backend.delete_model("model-a") | ||
|
|
||
| shutdown.assert_called_once_with() | ||
| assert backend._dispatch is None | ||
| assert backend._base_lora_signature is None | ||
| backend._inference_state_publisher.assert_called_once_with(None) | ||
|
|
||
|
|
||
| def test_fft_unload_tears_down_even_when_warm_enabled(): | ||
| backend = _backend(keep_runtime_warm=True, lora=False) | ||
|
|
||
| with patch("skyrl.backends.skyrl_train_backend.ray.shutdown") as shutdown: | ||
| backend.delete_model("model-a") | ||
|
|
||
| shutdown.assert_called_once_with() | ||
| assert backend._dispatch is None | ||
|
|
||
|
|
||
| def test_delete_unloads_synced_adapter_from_inference_engines(): | ||
| backend = _backend(keep_runtime_warm=True) | ||
| backend._inference_adapter_ids.add("model-a") | ||
|
|
||
| backend.delete_model("model-a") | ||
|
|
||
| backend._inference_engine_client.unload_lora_adapter.assert_awaited_once_with("model-a") | ||
| assert backend._inference_adapter_ids == set() | ||
|
|
||
|
|
||
| def test_delete_skips_inference_unload_for_unsynced_adapter(): | ||
| backend = _backend(keep_runtime_warm=True) | ||
|
|
||
| backend.delete_model("model-a") | ||
|
|
||
| backend._inference_engine_client.unload_lora_adapter.assert_not_awaited() | ||
|
|
||
|
|
||
| def test_delete_proceeds_when_inference_unload_fails(): | ||
| backend = _backend(keep_runtime_warm=True) | ||
| backend._inference_adapter_ids.add("model-a") | ||
| backend._inference_engine_client.unload_lora_adapter.side_effect = RuntimeError("vLLM unreachable") | ||
|
|
||
| backend.delete_model("model-a") | ||
|
|
||
| backend._dispatch.delete_adapter.assert_called_once_with("policy", "model-a") | ||
| assert backend._model_ids_to_role == {} | ||
| assert backend._inference_adapter_ids == set() | ||
|
|
||
|
|
||
| def test_create_model_registers_fresh_adapter_against_warm_runtime(): | ||
| backend = _backend(keep_runtime_warm=True, model_ids=()) | ||
|
|
||
| backend.create_model("model-b", LORA_CONFIG) | ||
|
|
||
| backend._dispatch.register_adapter.assert_called_once_with("policy", "model-b") | ||
| assert backend._model_ids_to_role == {"model-b": "policy"} | ||
|
|
||
|
|
||
| def test_create_model_signature_mismatch_rejected_against_warm_runtime(): | ||
| backend = _backend(keep_runtime_warm=True, model_ids=()) | ||
|
|
||
| with pytest.raises(ValueError, match="LoRA signature mismatch"): | ||
| backend.create_model("model-b", types.LoraConfig(rank=16, alpha=32, seed=0)) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
should we allow users to set this?