Skip to content

Revert D113962248: Optimizer scratch-buffer stashing in MemoryStashingManager (#4495) - #4495

Open
doIIarplus wants to merge 1 commit into
meta-pytorch:mainfrom
doIIarplus:export-D114959521
Open

Revert D113962248: Optimizer scratch-buffer stashing in MemoryStashingManager (#4495)#4495
doIIarplus wants to merge 1 commit into
meta-pytorch:mainfrom
doIIarplus:export-D114959521

Conversation

@doIIarplus

@doIIarplus doIIarplus commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary:

D113962248 added the torchrec half of "free DistributedShampoo scratch buffers
during the optimizer-state stash window": the _ScratchBufferOptimizer protocol,
scratch_buffers() discovery, _release_optimizer_scratch_buffers, a second
_optimizer_scratch_buffer_restore_callbacks list, the
restore_optimizer_state(restore_scratch_buffer=...) parameter, and
scratch_buffers() fan-out through CombinedOptimizer / KeyedOptimizerWrapper
/ OptimizerWrapper / SemiSyncOptimizer.

None of that machinery is needed. _global_dist_buffer is pure scratch that
DistributedShampoo allocates, fills, and consumes entirely within
update_params(); it never leaves the optimizer and is not part of state_dict.
Routing its free/re-alloc through torchrec meant a cross-repo protocol plus
wrapper fan-out and a parallel callback list, purely so an external caller could
drive a resize the owner could do itself.

Reverting so the feature can be re-implemented inside
hpc/optimizers/distributed_shampoo/dev/ behind
DDPDistributedConfig.free_dist_buffer_between_steps.

Middle of a 3-diff revert stack (on top of the D113962249 revert).

Two deliberate carve-outs -- this is NOT a mechanical sl backout:

  1. Kept the BUCK dep fix. D113962248 also changed //caffe2:_torch ->
    fbsource//third-party/pypi/torch:torch in torchrec/optim/BUCK and
    torchrec/optim/tests/BUCK. _torch is a private target that should not be
    depended on directly, so those hunks are left in place.

  2. Kept the staged_cpu_view_for contiguity fix and its TestCheckpointWhileStashed
    tests. D113962248 also replaced the view.set_(storage, offset, shape, stride)
    reconstruction with a contiguous reshape/narrow/view. That is an unrelated
    correctness fix: chunked_copy_ fills the pinned buffer in logical/row-major
    order, so rebuilding the view with the source's original (possibly transposed)
    stride hands the DCP stager transposed values -> silently corrupt optimizer
    state on resume. Reverting it would reintroduce that bug.

Original commit changeset: 37123fd3d19f
Original Phabricator Diff: D113962248

Reviewed By: wz337, hjmshi

Differential Revision: D114959521

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 5, 2026
@meta-codesync

meta-codesync Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@doIIarplus has exported this pull request. If you are a Meta employee, you can view the originating Diff in D114959521.

@wz337 wz337 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.

Review automatically exported from Phabricator review in Meta.

…gManager (meta-pytorch#4495)

Summary:

D113962248 added the torchrec half of "free DistributedShampoo scratch buffers
during the optimizer-state stash window": the `_ScratchBufferOptimizer` protocol,
`scratch_buffers()` discovery, `_release_optimizer_scratch_buffers`, a second
`_optimizer_scratch_buffer_restore_callbacks` list, the
`restore_optimizer_state(restore_scratch_buffer=...)` parameter, and
`scratch_buffers()` fan-out through `CombinedOptimizer` / `KeyedOptimizerWrapper`
/ `OptimizerWrapper` / `SemiSyncOptimizer`.

None of that machinery is needed. `_global_dist_buffer` is pure scratch that
DistributedShampoo allocates, fills, and consumes entirely within
`update_params()`; it never leaves the optimizer and is not part of `state_dict`.
Routing its free/re-alloc through torchrec meant a cross-repo protocol plus
wrapper fan-out and a parallel callback list, purely so an external caller could
drive a resize the owner could do itself.

Reverting so the feature can be re-implemented inside
`hpc/optimizers/distributed_shampoo/dev/` behind
`DDPDistributedConfig.free_dist_buffer_between_steps`.

Middle of a 3-diff revert stack (on top of the D113962249 revert).

Two deliberate carve-outs -- this is NOT a mechanical `sl backout`:

1. Kept the BUCK dep fix. D113962248 also changed `//caffe2:_torch` ->
   `fbsource//third-party/pypi/torch:torch` in `torchrec/optim/BUCK` and
   `torchrec/optim/tests/BUCK`. `_torch` is a private target that should not be
   depended on directly, so those hunks are left in place.

2. Kept the `staged_cpu_view_for` contiguity fix and its `TestCheckpointWhileStashed`
   tests. D113962248 also replaced the `view.set_(storage, offset, shape, stride)`
   reconstruction with a contiguous `reshape/narrow/view`. That is an unrelated
   correctness fix: `chunked_copy_` fills the pinned buffer in logical/row-major
   order, so rebuilding the view with the source's original (possibly transposed)
   stride hands the DCP stager transposed values -> silently corrupt optimizer
   state on resume. Reverting it would reintroduce that bug.

Original commit changeset: 37123fd3d19f
Original Phabricator Diff: D113962248

Reviewed By: wz337, hjmshi

Differential Revision: D114959521
@meta-codesync meta-codesync Bot changed the title Revert D113962248: Optimizer scratch-buffer stashing in MemoryStashingManager Revert D113962248: Optimizer scratch-buffer stashing in MemoryStashingManager (#4495) Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants