Skip to content

Commit 2c0ad42

Browse files
jeremyfowersclaude
andcommitted
fix(api): report the pin a load would actually use
Eighth review pass: - effective.pinned reported the saved value, but Router::load_model keeps a live process's own pin instead, so the field contradicted both reality and its documented meaning. It now reports the live pin when the model is loaded. - DELETE no longer resets a pin that was never saved; a pin a /load request applied to the process is not part of the entry being erased. - Validation errors no longer name the config.json section for an endpoint whose keys are recipe option names. - The user-model test cleans up its recipe_options.json entry, and the round-trip test covers a second recipe so options with no global-config counterpart are exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 8d30389 commit 2c0ad42

2 files changed

Lines changed: 74 additions & 18 deletions

File tree

src/cpp/server/server.cpp

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2828,11 +2828,20 @@ static std::string validate_option_value(const RuntimeConfig& config,
28282828
if (config_key.rfind(recipe_prefix, 0) == 0) {
28292829
config_key = config_key.substr(recipe_prefix.size());
28302830
}
2831+
const std::string section = RuntimeConfig::recipe_to_config_section(recipe);
28312832
try {
2832-
config.validate_backend(RuntimeConfig::recipe_to_config_section(recipe), config_key, value);
2833+
config.validate_backend(section, config_key, value);
28332834
} catch (const std::exception& e) {
2834-
const std::string message = e.what();
2835-
if (message.rfind("Unknown key:", 0) != 0) return message;
2835+
std::string message = e.what();
2836+
if (message.rfind("Unknown key:", 0) == 0) return "";
2837+
// The validator names keys as they appear in config.json; this endpoint
2838+
// speaks recipe option names, so drop the section qualifier.
2839+
const std::string qualifier = "'" + section + ".";
2840+
const auto at = message.find(qualifier);
2841+
if (at != std::string::npos) {
2842+
message.replace(at, qualifier.size(), "'" + key.substr(0, key.size() - config_key.size()));
2843+
}
2844+
return message;
28362845
}
28372846
return "";
28382847
}
@@ -2888,11 +2897,21 @@ void Server::respond_with_model_options(
28882897
without_saved.recipe_options = model_manager_->get_model_default_options(info);
28892898
RecipeOptions defaults = router_->resolve_effective_options(without_saved, no_request_options);
28902899

2900+
nlohmann::json effective_json = resolve_all_recipe_options(effective);
2901+
if (router_->is_model_loaded(model_key)) {
2902+
// A load keeps a live process's own pin rather than the saved one
2903+
// (see Router::load_model), so reporting the saved value here would
2904+
// contradict what "what a load would use" means for every other key.
2905+
const auto live_pinned = router_->get_model_recipe_options(model_key)
2906+
.get_option("pinned");
2907+
effective_json["pinned"] = live_pinned.is_boolean() && live_pinned.get<bool>();
2908+
}
2909+
28912910
nlohmann::json response = {
28922911
{"model_name", model_id},
28932912
{"recipe", info.recipe},
28942913
{"saved", model_manager_->get_saved_model_options(model_key)},
2895-
{"effective", resolve_all_recipe_options(effective)},
2914+
{"effective", std::move(effective_json)},
28962915
{"defaults", resolve_all_recipe_options(defaults)},
28972916
{"reload_required", router_->would_reload(model_key, effective)}
28982917
};
@@ -2985,9 +3004,10 @@ void Server::handle_model_options_delete(const httplib::Request& req, httplib::R
29853004
respond_with_model_options(req, res,
29863005
[this](const std::string& model_key, const ModelInfo&, httplib::Response&,
29873006
bool& touched_pinned) {
2988-
// Resetting to defaults covers the pin whether or not one was saved:
2989-
// a request-scoped /load may have pinned the live process instead.
2990-
touched_pinned = true;
3007+
// Only a saved pin is this endpoint's to reset. A pin a /load
3008+
// request applied to the live process is not part of the entry
3009+
// being erased, and `effective` reports it either way.
3010+
touched_pinned = model_manager_->get_saved_model_options(model_key).contains("pinned");
29913011
if (!model_manager_->get_saved_model_options(model_key).empty()) {
29923012
model_manager_->set_saved_model_options(model_key, nlohmann::json::object());
29933013
}

test/server_endpoints.py

Lines changed: 47 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1344,6 +1344,30 @@ def live_pinned():
13441344
self._reset_options()
13451345
self.assertFalse(live_pinned(), "DELETE should unpin the live model")
13461346

1347+
# A pin applied by a /load request is not saved anywhere, but a load
1348+
# would keep it, so `effective` has to report it rather than the saved
1349+
# value, and DELETE must not reset something it never owned.
1350+
requests.post(
1351+
f"{self.base_url}/load",
1352+
json={"model_name": ENDPOINT_TEST_MODEL, "pinned": True},
1353+
timeout=TIMEOUT_MODEL_OPERATION,
1354+
)
1355+
reported = requests.get(self._options_url(), timeout=TIMEOUT_DEFAULT).json()
1356+
self.assertEqual(reported["saved"], {})
1357+
self.assertTrue(
1358+
reported["effective"]["pinned"],
1359+
"effective must report the live pin, which is what a load would keep",
1360+
)
1361+
self._reset_options()
1362+
self.assertTrue(
1363+
live_pinned(), "DELETE must not reset a pin that was never saved"
1364+
)
1365+
requests.post(
1366+
f"{self.base_url}/load",
1367+
json={"model_name": ENDPOINT_TEST_MODEL, "pinned": False},
1368+
timeout=TIMEOUT_MODEL_OPERATION,
1369+
)
1370+
13471371
print("[OK] pinned changes are applied to the running model")
13481372

13491373
def test_012x_every_effective_option_can_be_saved_back(self):
@@ -1356,17 +1380,26 @@ def test_012x_every_effective_option_can_be_saved_back(self):
13561380
self.addCleanup(self._reset_options)
13571381
self._reset_options()
13581382

1359-
effective = requests.get(self._options_url(), timeout=TIMEOUT_DEFAULT).json()[
1360-
"effective"
1361-
]
1362-
response = requests.post(
1363-
self._options_url(), json=effective, timeout=TIMEOUT_DEFAULT
1364-
)
1365-
self.assertEqual(
1366-
response.status_code,
1367-
200,
1368-
f"Every reported option must be settable, got {response.text}",
1369-
)
1383+
# Both recipes, so the round trip covers options with a global-config
1384+
# counterpart (steps, cfg_scale) and options without one (flow_shift,
1385+
# sampling_method, merge_args, the eviction settings).
1386+
models = [ENDPOINT_TEST_MODEL]
1387+
if requests.get(f"{self.base_url}/models/SD-Turbo", timeout=TIMEOUT_DEFAULT).ok:
1388+
models.append("SD-Turbo")
1389+
self.addCleanup(self._reset_options, "SD-Turbo")
1390+
1391+
for model in models:
1392+
effective = requests.get(
1393+
self._options_url(model=model), timeout=TIMEOUT_DEFAULT
1394+
).json()["effective"]
1395+
response = requests.post(
1396+
self._options_url(model=model), json=effective, timeout=TIMEOUT_DEFAULT
1397+
)
1398+
self.assertEqual(
1399+
response.status_code,
1400+
200,
1401+
f"Every option reported for {model} must be settable, got {response.text}",
1402+
)
13701403

13711404
# merge_args specifically: a boolean, and both values have to round-trip.
13721405
for value in (True, False):
@@ -1436,6 +1469,9 @@ def test_012v_options_accept_a_user_model_public_name(self):
14361469
).json()["checkpoint"]
14371470

14381471
def cleanup():
1472+
# Options first: deleting the model leaves its recipe_options.json
1473+
# entry behind otherwise.
1474+
self._reset_options(model_name)
14391475
requests.post(
14401476
f"{self.base_url}/unload",
14411477
json={"model_name": model_name},

0 commit comments

Comments
 (0)