Skip to content

[Dev] Add combined 1F1B schedule-aware tensor release - #7062

Open
lhb8125 wants to merge 18 commits into
NVIDIA:devfrom
lhb8125:hongbinl/tensor-bound-scheduled-lifetime
Open

[Dev] Add combined 1F1B schedule-aware tensor release#7062
lhb8125 wants to merge 18 commits into
NVIDIA:devfrom
lhb8125:hongbinl/tensor-bound-scheduled-lifetime

Conversation

@lhb8125

@lhb8125 lhb8125 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Track producer streams on concrete tensor objects within each combined 1F1B model-chunk plan.
  • Consume each binding at the next real node: free_input=False only drops ownership metadata, while external or unregistered tensors retain the conservative record_stream fallback.
  • Export tensors that cross plan boundaries and drain deferred releases during phase finalization, with an audit for dangling owner bindings.
  • Reject all storage aliases in managed node transitions so object-level ownership and EMPTY_STORAGE remain unambiguous.
  • Keep the detailed lifetime-management documentation next to the implementation in combined_1f1b_tensor_release.py, and scope implementation names to this schedule.

This supersedes #6948 with the reviewed tensor-bound ownership model.

Test plan

  • Project Python py_compile on all 12 changed Python files.
  • CHECK_ONLY=true BASE_REF=dev bash tools/autoformat.sh (Black, isort, Pylint, and Ruff pass).
  • python tools/check_copyright.py on the complete PR changeset.
  • CMH GB200 CI container: tests/unit_tests/a2a_overlap/test_combined_1f1b_tensor_release.py21 passed in 4.52s.
  • The allocator functional test compares identical cross-stream dependencies and verifies scheduled release reuses the owner allocation and lowers peak reserved memory versus record_stream while the consumer is still in flight. It takes 0.12s; total file time increased from 4.41s to 4.52s.

@copy-pr-bot

copy-pr-bot Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
@lhb8125
lhb8125 force-pushed the hongbinl/tensor-bound-scheduled-lifetime branch from 2a20947 to ff310ef Compare September 3, 2026 15:28
Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
Signed-off-by: hongbinl <hongbinl@nvidia.com>
@lhb8125 lhb8125 changed the title [Dev] Add tensor-bound schedule-aware tensor lifetime management [Dev] Add combined 1F1B schedule-aware tensor release Sep 4, 2026
Signed-off-by: hongbinl <hongbinl@nvidia.com>
@lhb8125
lhb8125 marked this pull request as ready for review September 8, 2026 03:44
@lhb8125
lhb8125 requested review from a team as code owners September 8, 2026 03:44
@lhb8125
lhb8125 requested a review from Wohox September 8, 2026 03:44
Comment thread megatron/core/pipeline_parallel/combined_1f1b_tensor_release.py
Signed-off-by: hongbinl <hongbinl@nvidia.com>
in 1f1b phase of pipelining or non-pipelining schedule.
"""

ep_overlap_use_scheduled_tensor_release: bool = False

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about setting to True by default? Maybe we need more benchmarking data to support this comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I am keeping this opt-in for now because the current evidence covers correctness and a native-allocator microbenchmark, but not end-to-end throughput and peak memory across the supported EP-overlap configurations. This knob is only useful for the sync-free combined 1F1B path with overlap_moe_expert_parallel_comm=True; other schedules can keep the default allocator record_stream path. Also, changing this field to True alone would make the default TransformerConfig invalid because validation requires the parent overlap flag. I clarified when users should enable the knob in 188e5f5b4. We can revisit a conditional default-on behavior, with an explicit opt-out, after representative on/off benchmarks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This knob only helps the sync-free path, which is not widely used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, but for non sync-free path where CPU is faster like DSv4/3 case this toggle should also help?

@lhb8125

lhb8125 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 930c99c

Signed-off-by: hongbinl <hongbinl@nvidia.com>
@lhb8125

lhb8125 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 3a8b253

Signed-off-by: hongbinl <hongbinl@nvidia.com>
@lhb8125

lhb8125 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 7dc1595

@Wohox Wohox left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more comments, LGTM!

@lhb8125
lhb8125 enabled auto-merge September 11, 2026 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants