fix(registry): merge duplicate model deployments instead of shadowing (#3709)#4316
Open
nac7 wants to merge 2 commits into
Open
fix(registry): merge duplicate model deployments instead of shadowing (#3709)#4316nac7 wants to merge 2 commits into
nac7 wants to merge 2 commits into
Conversation
normalize_text removes English articles ("a", "an", "the") before
computing F1. When the gold or pred string consists entirely of such
a word -- for example, the letter "A" in MMLU-style multiple-choice
benchmarks -- article removal empties the token set. f_measure on an
empty set returns None, which f1_score silently converts to 0.0, so a
correct prediction scores 0 instead of 1.
Fix: after normalization, if either token set is empty, fall back to
normalization with should_remove_articles=False before calling
f_measure. Multi-word strings that contain articles alongside other
tokens are unaffected.
Adds test_evaluate_reference_metrics.py coverage for f1_score:
- test_f1_score_normal (baseline behavior unchanged)
- test_f1_score_article_gold (regression: "A"/"An"/"The" as gold)
- test_f1_score_article_in_longer_string (article removal still fires
for multi-word strings)
- test_f1_score_empty_pred (empty prediction still yields 0)
Fixes stanford-crfm#2298
PR stanford-crfm#3694 changed the deployment resolution order so that prod_env entries are registered after built-in (helm.config) entries and therefore win the [-1] pick in get_default_model_deployment_for_model. This means a prod_env/model_deployments.yaml entry that is incomplete (e.g. tokenizer_name omitted because the user only wanted to change client_spec) silently shadows the full built-in entry. The WindowService then receives tokenizer_name=None and raises ValueError. Fix: when register_model_deployment encounters a name that is already in DEPLOYMENT_NAME_TO_MODEL_DEPLOYMENT, build a merged ModelDeployment: - the new entry's non-None field values override the existing ones - None fields in the new entry fall back to the existing entry's values - name, client_spec, and deprecated are always taken from the new entry A hwarn is emitted whenever a merge occurs so that users can see which sources are competing. Adds test_model_deployment_registry.py with six tests covering: - first registration stored as-is - full override replaces all fields - sparse override inherits None fields from built-in (regression stanford-crfm#3709) - sparse override inherits max_sequence_length from built-in - deprecated flag always taken from the new entry - dict holds exactly one entry after override Fixes stanford-crfm#3709
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
PR #3694 changed the deployment resolution order so that
prod_env/model_deployments.yamlentries are registered after the built-in
helm.configentries, andget_default_model_deployment_for_modelpicks[-1](last registered) asthe default.
As a result, a
prod_enventry that is incomplete — e.g. a user whoonly wanted to change
client_specbut lefttokenizer_nameunset —silently shadows the full built-in entry. The
WindowServicethenreceives
tokenizer_name=Noneand raises:The confusing part: the user's config file is correct and a standalone
diagnostic script proves it loads fine, but
helm-runstill fails becausethe
prod_enventry wins.Fix
src/helm/benchmark/model_deployment_registry.py—register_model_deploymentWhen a deployment name is already present in
DEPLOYMENT_NAME_TO_MODEL_DEPLOYMENT,merge the new entry over the existing one rather than blindly appending:
Nonefields override the existing values (preserving the intended prod_env-wins behaviour from Change logic for default model deployments #3694).Nonein the new entry fall back to the existing value, so a sparseprod_enventry cannot accidentally wipetokenizer_name(ormax_sequence_length, etc.) that was set in the built-in config.name,client_spec, anddeprecatedare always taken from the new entry.hwarnis emitted on every merge so that users can see which sources are competing.Tests
src/helm/benchmark/test_model_deployment_registry.py(new file, 6 tests):test_first_registration_stored_as_istest_override_with_full_entry_replaces_all_fieldstest_sparse_override_inherits_none_fields_from_builtintokenizer_name=Nonein prod_env falls back to built-in valuetest_sparse_override_none_max_sequence_inherits_from_builtinmax_sequence_length=Nonein prod_env falls back to built-intest_deprecated_always_taken_from_new_entrydeprecatedbool always uses new entrytest_only_one_entry_in_lookup_dict_after_overrideFixes #3709