fix: stamp response with sampler version atomically with the client - #2
Open
golldyck wants to merge 1 commit into
Open
fix: stamp response with sampler version atomically with the client#2golldyck wants to merge 1 commit into
golldyck wants to merge 1 commit into
Conversation
_route_to_tinker read the active sampling client (get_sampling_client) and its version stamp (get_sampler_version) in two separate lock acquisitions, with prompt rendering and SamplingParams construction between them. A concurrent set_sampling_client(new, version+1) — the trainer's between-step hot-swap — landing in that window stamped the response with a version that did not produce the sampled tokens, so the trainer's staleness = save_count - sampler_version diagnostic (the HF BF16-mismatch measurement the stamp exists for) under-counted drift. Add get_sampling_client_and_version(), which reads both under one lock, and capture (client, version) together at the top of _route_to_tinker before rendering. The existing getters stay for compatibility. Test: a hot-swap injected inside the render step (the exact race window, which is synchronous so no asyncio task can preempt it) now leaves the stamped sampler_version matching the client that served the call. The test fails on the two-lock code and passes on the snapshot helper.
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
_route_to_tinkerintinker_nemogym/tinker_responses_model.pynow captures the active sampling client and its version stamp in a single lock acquisition. That guarantees thesampler_versionwritten onto every response is the version of the client that actually produced the tokens.Cause
The routing helper read the two values in two separate lock acquisitions:
client = get_sampling_client()at the top of the call (~line 172), thensampler_version_at_call = get_sampler_version()just before sampling (~line 228).Between those reads it renders the prompt and builds
SamplingParams. The trainer hot-swaps the sampler between RL steps viaset_sampling_client(new_client, save_count + 1). If that swap lands in the window between the two reads, the response is stamped with the new version while the old client (correctly captured by reference) serves the call.The trainer then computes
staleness = save_count - sampler_version(the HuggingFace BF16-mismatch drift diagnostic the version stamp exists for) and under-counts staleness. This is the in-flight hot-swap scenario the surrounding comments say they guard against.Fix
get_sampling_client_and_version(), which returns(client, version)read under one acquisition of_sampling_client_lock, a consistent snapshot._route_to_tinker, captureclient, sampler_version_at_call = get_sampling_client_and_version()up front (before rendering) and drop the later secondget_sampler_version()read.get_sampling_client()/get_sampler_version()getters are kept unchanged for compatibility. The helper is added to__all__.Test
tests/unit/test_tinker_responses_model.py:test_get_sampling_client_and_version_reads_atomicallychecks the combined getter returns a consistent snapshot.test_route_stamps_version_of_client_that_served_across_hot_swapinjects the trainer'sset_sampling_client(B, v+1)swap inside the race window (fromrenderer.build_generation_prompt, which runs between the two reads; the window is synchronous, so noasynciotask can preempt it). It asserts client A still served the call and the stampedsampler_versionis A's version (v), not B's (v+1).The mid-route test fails on the two-lock code (
assert 8 == 7) and passes with the snapshot helper. Verified under python3.11 with fastapi/pydantic/pytest/pytest-asyncio and a stubbedtinker.