ci: Bump Megatron-Bridge to 5ed9799 - #4022
Conversation
Signed-off-by: Cory Ye <cye@nvidia.com>
Signed-off-by: nemo-ci-bot <nemo-ci-bot@nvidia.com>
…blocker. Signed-off-by: Cory Ye <cye@nvidia.com>
Signed-off-by: Cory Ye <cye@nvidia.com>
Signed-off-by: Cory Ye <cye@nvidia.com>
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
2c27735 to
16180e3
Compare
Signed-off-by: Cory Ye <cye@nvidia.com>
|
/ok to test 9e91bbd |
|
/ok to test 9e91bbd |
|
Superseded by a newer MBridge bump PR. |
|
/ok to test e9f51b0 |
e9f51b0 to
b997d39
Compare
|
/ok to test b997d39 |
b997d39 to
b99ee92
Compare
|
/ok to test b99ee92 |
Signed-off-by: Cory Ye <cye@nvidia.com>
b99ee92 to
571229c
Compare
|
/ok to test 571229c |
Signed-off-by: Cory Ye <cye@nvidia.com>
|
Ran the last test case in L1 single controller manually (Hydra config |
| # Adoption occurs only at HTTP startup, after model initialization | ||
| # can no longer leak the listener into long-lived child processes. | ||
| assert holder._sock.fileno() == -1 | ||
| assert reserved_socket.getsockname()[1] == port |
There was a problem hiding this comment.
1 action item.
TL;DR — the rewrite dropped the post-adoption create_connection, which was the only assertion that tested whether the port is still accepting connections after the handoff — the exact property the test's docstring says it exists to check.
The test states the contract about itself, in its docstring at L169-L175:
the worker adopts that same socket through the fd handoff — the port is never released in between — and the server falls back to a fresh port only when nothing was reserved.
Nothing left in the test checks the middle clause. At the base commit it was checked by connecting to the port after adoption, with the holder's own copy already closed — test_megatron_generation_parse.py#L205-L209 @ a366bc8 (linked at the base, since these lines no longer exist at head):
assert holder._sock.fileno() == -1
assert reserved.getsockname()[1] == port
with socket.create_connection(("127.0.0.1", port), timeout=5):
passThe rewrite keeps the first two lines and drops the connect. Those two do not cover it. holder._sock.fileno() == -1 proves the holder closed its copy. getsockname()[1] == port proves the adopted object is bound to the right port. Neither proves the socket is still listening and accepting — a socket can be bound with no live accept queue. "Never released" is a property about the port still being alive, and the connect was the only assertion that tested that.
The remaining connect at L191-L194 runs before adoption, so it only proves the holder was listening.
The timing of that handoff is what this PR changes, and this test is what guards the change — which makes this the wrong assertion to lose in this PR specifically.
AI-1
| assert reserved_socket.getsockname()[1] == port | |
| assert reserved_socket.getsockname()[1] == port | |
| # Still accepting after the holder closed its copy: the port was | |
| # never released across the handoff. | |
| with socket.create_connection(("127.0.0.1", port), timeout=5): | |
| pass |
Restoring the connect is safe: start_text_gen_server is monkeypatched to a lambda that only records its kwargs, so nothing closes the socket during the test and the connection will succeed.
Not run locally — this file is @pytest.mark.mcore and needs a GPU/mcore environment, which the review side does not have.
There was a problem hiding this comment.
This is a good catch, technically this socket should be holding onto the port as soon as HeldPortReservation() is initialized, so nothing else takes it, but low chance of anything stealing this port in a unit test env.
The worker adopted the driver-reserved listening socket in __init__, so a live listening fd existed for the whole of model initialization. Anything forked during init inherited a duplicate, and because the frontends bind with SO_REUSEPORT, the kernel could route connections to a child that never accepts HTTP requests -- an extra no-op endpoint from Gym's point of view. Keep only the port number through init and adopt the socket in _setup_openai_api_server, immediately before the frontends start. The port holder keeps it bound and listening until then, so the pre-published URL still cannot be stolen and early Gym probes still queue rather than being refused. Ports as reserved per rank, so this defers the handoff for every frontend the driver reserved, not just one. Mirrors the same fix upstream in NVIDIA-NeMo#4022 (b38c38f), adapted to the per-frontend reservation map on this branch. Signed-off-by: Siddharth Singh <sidsingh@nvidia.com>
The worker adopted the driver-reserved listening socket in __init__, so a live listening fd existed for the whole of model initialization. Anything forked during init inherited a duplicate, and because the frontends bind with SO_REUSEPORT, the kernel could route connections to a child that never accepts HTTP requests -- an extra no-op endpoint from Gym's point of view. Keep only the port number through init and adopt the socket in _setup_openai_api_server, immediately before the frontends start. The port holder keeps it bound and listening until then, so the pre-published URL still cannot be stolen and early Gym probes still queue rather than being refused. Ports as reserved per rank, so this defers the handoff for every frontend the driver reserved, not just one. Mirrors the same fix upstream in NVIDIA-NeMo#4022 (b38c38f), adapted to the per-frontend reservation map on this branch. Signed-off-by: Siddharth Singh <sidsingh@nvidia.com>
The worker adopted the driver-reserved listening socket in __init__, so a live listening fd existed for the whole of model initialization. Anything forked during init inherited a duplicate, and because the frontends bind with SO_REUSEPORT, the kernel could route connections to a child that never accepts HTTP requests -- an extra no-op endpoint from Gym's point of view. Keep only the port number through init and adopt the socket in _setup_openai_api_server, immediately before the frontends start. The port holder keeps it bound and listening until then, so the pre-published URL still cannot be stolen and early Gym probes still queue rather than being refused. Ports as reserved per rank, so this defers the handoff for every frontend the driver reserved, not just one. Mirrors the same fix upstream in NVIDIA-NeMo#4022 (b38c38f), adapted to the per-frontend reservation map on this branch. Signed-off-by: Siddharth Singh <sidsingh@nvidia.com>
The worker adopted the driver-reserved listening socket in __init__, so a live listening fd existed for the whole of model initialization. Anything forked during init inherited a duplicate, and because the frontends bind with SO_REUSEPORT, the kernel could route connections to a child that never accepts HTTP requests -- an extra no-op endpoint from Gym's point of view. Keep only the port number through init and adopt the socket in _setup_openai_api_server, immediately before the frontends start. The port holder keeps it bound and listening until then, so the pre-published URL still cannot be stolen and early Gym probes still queue rather than being refused. Ports as reserved per rank, so this defers the handoff for every frontend the driver reserved, not just one. Mirrors the same fix upstream in NVIDIA-NeMo#4022 (b38c38f), adapted to the per-frontend reservation map on this branch. Signed-off-by: Siddharth Singh <sidsingh@nvidia.com>
The worker adopted the driver-reserved listening socket in __init__, so a live listening fd existed for the whole of model initialization. Anything forked during init inherited a duplicate, and because the frontends bind with SO_REUSEPORT, the kernel could route connections to a child that never accepts HTTP requests -- an extra no-op endpoint from Gym's point of view. Keep only the port number through init and adopt the socket in _setup_openai_api_server, immediately before the frontends start. The port holder keeps it bound and listening until then, so the pre-published URL still cannot be stolen and early Gym probes still queue rather than being refused. Ports as reserved per rank, so this defers the handoff for every frontend the driver reserved, not just one. Mirrors the same fix upstream in NVIDIA-NeMo#4022 (b38c38f), adapted to the per-frontend reservation map on this branch. Signed-off-by: Siddharth Singh <sidsingh@nvidia.com>
3 participants
What does this PR do ?
Replaces: #3991
mainbranch...).bumpA seg-faulting log prob test case for Megatron inference (PP > 2 and async scheduling).bumpDeactivatecpu_initialization, which is a new default from Megatron-Bridge.mainRelax aNotImplementedErrorfrom da39e17.bumpFix a dopple-ganger socket forked (via async checkpointing) from a port reservation that is stealing Megatron inference requests with the new replicate inference servers for Megatron-Inference.mainFix a Gym regression introduced by 7036e5d.Details
HeldPortReservation. We hand the port reserved by the socket to Gym.HeldPortReservationclosing the original one doesn't close the second one, which ends up being a dopple-ganger of sorts.async_save=True.Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information