Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
| self.assertEqual(scheduler._fpm_publisher.dp_rank, 2) | ||
| self.assertTrue(scheduler._fpm_publisher.endpoint.startswith("ipc://")) | ||
| self.assertIsNotNone(scheduler.server_args.forward_pass_metrics_ipc_name) | ||
| self.assertIsNotNone( |
There was a problem hiding this comment.
[suggestion] The assertion that the generated endpoint appears in get_context().resolved_server_args_dict()["forward_pass_metrics_ipc_name"] is necessary for the external-consumer contract, but it is weak as a regression guard for how the value was published. resolved_server_args_dict is vars(server_args) overlaid with _overrides_log. A regression that wrote only via server_args.override(...) (instance mutation, no bag write) would still make the key non-None through the vars path, so this assert alone would stay green. The writer ratchet is the real fence against reintroducing the ServerArgs.override call site; the unit test does not independently pin “bags written, seed pristine.”
Suggestion: Strengthen the case that must go red if the write path regresses off bags, e.g. assert both of:
d72e17e to
595924b
Compare
595924b to
a489a5f
Compare
Re-review (Round 2 —
|
a489a5f to
03e797b
Compare
03e797b to
d959e89
Compare
d959e89 to
8df9807
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8df9807256
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8df9807 to
7ef5e92
Compare
|
Round 5 — the observation is correct but it is not a regression from this PR, so I am leaving it. @chatgpt-codex-connector is right that with Fixing it properly means either propagating the endpoint from the last stage to the rank that serves the readback, or deriving it deterministically instead of from |
Outcome
Checks
|
7ef5e92 to
76bbbe2
Compare
76bbbe2 to
626e102
Compare
626e102 to
78cfa10
Compare
… the bags When --forward-pass-metrics-ipc-name is left unset the reporter generates an endpoint and has to hand it to external consumers (the documented contract is that they read it back from the server config). That readback is the scheduler's get_internal_state, which already reports get_context().resolved_server_args_dict(), so the write moves to get_context().override and the read alongside it to get_observability() — the endpoint still shows up in /server_info's internal_states, and the ServerArgs instance stops being a message bus. The test's server_args stand-in (a SimpleNamespace with a hand-rolled override) becomes a real published config, so the reporter exercises the same accessors as production. Writer ratchet 19 -> 18.
78cfa10 to
99abdd2
Compare
|
Closing unmerged and reopening against GitHub classifies a chained-base series as a stack, and in that mode it refuses base retargeting ( The replacement PR carries the identical commit; the review history, the six rounds of comment triage and the validation notes stay here for reference. Link posted below. |
|
Reopened as #33337 (base |
1 participant
What
When
--forward-pass-metrics-ipc-nameis left unset, the reporter generates anendpoint and has to hand it to external consumers; the documented contract is that
they read it back from the server config (
server_arguments.mdxpoints the Dynamoplanner at
server_args.forward_pass_metrics_ipc_name).That readback is the scheduler's
get_internal_state, which already reportsget_context().resolved_server_args_dict()— the pristine config with post-publishoverrides overlaid. So the write moves to
get_context().override(...)and the readnext to it to
get_observability(). The endpoint still appears in/server_info'sinternal_states, andServerArgsstops being used as a message bus between thereporter and its consumers.
Test changes
The test's
server_argsstand-in was aSimpleNamespacecarrying a hand-rolledoverride; it now publishes a real config, so the reporter goes through the sameaccessors as production, and the endpoint is asserted through the resolved config.
Validation
test/registered/unit/observabilityand the config ratchets pass; full registeredCPU battery shows no new failures against the base commit.
Writer ratchet 19 → 18.
CI States
Latest PR Test (Base): ❌ Run #30771345753
Latest PR Test (Extra): 🚫 Run #30784016246