Conversation
…nfig.from_args The dataclass declares static_model_labels, prefill_model_labels and decode_model_labels at lines 58, 63 and 64. The parser defines --static-model-labels, --prefill-model-labels and --decode-model-labels at lines 176, 432 and 439. reconfigure_service_discovery reads all three, at lines 185, 190 and 193. from_args assigns none of them, so all three stay None. app.py:260 builds this object as init_config and hands it to the watcher, which holds it as current_config until the config file first changes. main_router.py:242 serialises current_config into /health, so /health reports three fields as null that the operator set. parser.py:540 merges a dynamic config file into args as defaults before from_args runs, so the drop applies whether the values arrived on the command line or in that file. Two regression tests. The first asserts the three values survive from_args. The second asks, for every field the dataclass and the parser share, whether it moved off its dataclass default. Asking about the default rather than about equality with the parsed argument is deliberate, and two earlier versions of this test proved why. Comparing for equality passed for eight of twenty-one fields, because argparse and the dataclass are written to share their defaults, so both sides read the same even with the assignment deleted. Comparing for truthiness then still missed static_backend_health_check_interval and priority_header, whose defaults are themselves truthy. Deleting each of nine assignments in turn now fails the test in every case. Signed-off-by: chrikrah <48338417+chrikrah@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates DynamicRouterConfig.from_args to include static_model_labels, prefill_model_labels, and decode_model_labels from the parsed arguments, and introduces a new test suite to verify that no fields defined by the parser are dropped. The review feedback points out that --priority-field in the test's argument list is set to its default value, which prevents the parity test from detecting if it is dropped, and suggests changing it to a non-default value.
| "--priority-header", | ||
| "x-priority", | ||
| "--priority-field", | ||
| "priority", |
There was a problem hiding this comment.
The value for --priority-field in ARGV is set to "priority", which is identical to its default value in both the parser and the DynamicRouterConfig dataclass. As a result, if priority_field were to be dropped in from_args, this test would not detect it because getattr(args, name) != defaults[name] would evaluate to False (i.e., "priority" != "priority" is False). To ensure priority_field is properly covered by the parity test, change this value to a non-default string such as "custom-priority".
| "priority", | |
| "custom-priority", |
…arity test gemini-code-assist pointed out that --priority-field was set to "priority", which is its default in both the parser and the dataclass, so the parity check could not see that field: a dropped assignment leaves it reading the same on both sides. It is now request_priority. The test also asserts up front that no shared field sits at its dataclass default, so the next field added to ARGV without a distinct value fails here rather than passing quietly. That is the third form of this same blindness found in this test, after comparing for equality and comparing for truthiness. Deleting each of the eighteen assignments in from_args in turn now fails the test in every case. Signed-off-by: chrikrah <48338417+chrikrah@users.noreply.github.com>
|
Correct, and it was the third form of that blindness in this one test.
The test now asserts up front that no shared field sits at its dataclass default, so the next field added without a distinct value fails here rather than passing quietly. Deleting each of the eighteen assignments in |
| # log_stats_interval: int | ||
|
|
||
| @staticmethod | ||
| def from_args(args) -> "DynamicRouterConfig": |
There was a problem hiding this comment.
Optional simplification:
@staticmethod
def from_args(args) -> "DynamicRouterConfig":
return DynamicRouterConfig(
**{f.name: getattr(args, f.name) for f in dataclasses.fields(DynamicRouterConfig)}
)What do you think?
There was a problem hiding this comment.
Taken, and it is better than what I had: 23 lines out, 8 in, in 0944774.
I checked the assumption first. The dataclass declares 21 fields, and the real parser puts all 21 on its namespace. That holds on the minimal command line too, so no field arrives only because a flag was passed:
dataclass fields: 21
MISSING: []
MISSING (minimal ARGV): []
attrs the explicit from_args read: 21
fields NOT read today: []
old == new (test ARGV): True
I read that attribute list out of the source with ast, not by eye. parser.py uses no SUPPRESS and no subparsers, and its one mutually exclusive group holds no dataclass field. So no command line leaves one of the 21 off args.
233 passed.
One thing to be exact about. This commit changes no behaviour, so nothing fails when you remove it. The tests pin 2102e76, the missing-field fix. Revert to the parent of that commit and both cases fail:
FAILED src/tests/test_dynamic_config_from_args.py::test_from_args_carries_the_three_label_fields
FAILED src/tests/test_dynamic_config_from_args.py::test_from_args_drops_no_field_the_parser_also_defines
AssertionError: assert ['decode_mode...model_labels'] == []
Your version gets the class of defect rather than the instance. A field added to the dataclass and the parser now needs no second edit here. One added to the dataclass alone raises at this call instead of reading as null in /health.
pre-commit on the file: black, isort and codespell pass.
… a time Shaoting-Feng suggested building the object from dataclasses.fields instead of twenty-one explicit assignments, which is what let three of them go missing in the first place. Checked before applying it: the dataclass declares 21 fields, and the real parser puts all 21 on its namespace. It does so on the minimal command line as well as on the one the test uses, so no field arrives only when its flag is passed. The explicit version already read exactly those 21 attributes and no others, so the comprehension asks nothing of args that the old code did not, and both constructions compare equal. A field added to the dataclass and the parser is now carried without a second edit here. One added to the dataclass alone raises AttributeError at this call instead of reading as null in the /health payload. Signed-off-by: chrikrah <48338417+chrikrah@users.noreply.github.com>
|
The two red checks here are not from this change, and both are measurable.
#1093, #1095 and #1097 are mine, they are green on it, and #1094 touches one file,
Locally, |
This branch has not been deployed
2 participants
DynamicRouterConfigdeclaresstatic_model_labels(line 58),prefill_model_labels(line 63) anddecode_model_labels(line 64).parser.pydefines--static-model-labels(line 176),--prefill-model-labels(line 432) and--decode-model-labels(line 439).reconfigure_service_discoveryreads all three, at lines 185, 190 and 193.from_argsassigns none of them, so all three stayNone.On
ebbb862, withparse_args()over a static plus disaggregated command line, thenfrom_args:app.py:260builds this object asinit_configand hands it to the watcher, which keeps it ascurrent_configuntil the config file first changes.main_router.py:242serialisescurrent_configinto/health, so/healthreports three fields as null that the operator set.parser.py:540merges a dynamic config file intoargsas defaults beforefrom_argsruns, so the drop applies whether the values arrived on the command line or in that file.The fix is three added lines, beside the fields already assigned from the same
args.Two regression tests. The first asserts the three values survive
from_args. The second asks, for every field the dataclass and the parser share, whether it moved off its dataclass default.Asking about the default rather than about equality with the parsed argument is deliberate, and two earlier versions of that test proved why. Comparing for equality passed for eight of twenty-one fields, because argparse and the dataclass are written to share their defaults, so both sides read the same even with the assignment deleted. Comparing for truthiness then still missed
static_backend_health_check_intervalandpriority_header, whose defaults are themselves truthy.Deleting each of nine assignments in turn now fails the test in every case:
The default comparison also tolerates a transformation such as
parse_comma_separated_argsmoving intofrom_argslater: a transformed value still differs from the declared default.No open issue covers this, so there is nothing to link.
-swhen doinggit commit[Bugfix],[Feat], and[CI].Two things stay out of this change.
k8s_service_discovery_typeandk8s_watcher_timeout_secondsnever reach the dataclass at all, sofrom_argscannot drop them;app.py:228and:235pass both at startup while the k8s branch ofreconfigure_service_discoverypasses neither, which is a larger defect than this one and belongs on its own. And no live deployment exercised the reconfigure path, so this coversfrom_argsand the/healthpayload it feeds.