Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22857
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 Cancelled JobAs of commit 9d1ccdc with merge base 81a2379 ( CANCELLED JOB - The following job was cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
🟡 Changes recommended
The critical tensor validation issue remains unresolved, along with two nits.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a C++ AdamW optimizer with decoupled weight decay to ExecuTorch training.
Changes:
- Implements AdamW state, options, and updates.
- Registers library and test targets.
- Adds behavioral tests and documentation.
Review findings:
adamw.cpp:145, 152, 163— critical (1 vote): validate dtype/layout and matching shapes before updates.README.md:11— nit (3 votes): useC++instead ofCpp.adamw_test.cpp:88— nit (3 votes): add changing-gradient multi-step coverage with tighter expectations.
File summaries
| File | Summary |
|---|---|
shim_et/xplat/executorch/build/build_variables.bzl |
Registers the AdamW source. |
extension/training/README.md |
Documents AdamW availability. |
extension/training/optimizer/test/targets.bzl |
Defines the AdamW test target. |
extension/training/optimizer/test/adamw_test.cpp |
Adds AdamW unit tests. |
extension/training/optimizer/targets.bzl |
Defines the AdamW library target. |
extension/training/optimizer/adamw.h |
Declares the AdamW API and state. |
extension/training/optimizer/adamw.cpp |
Implements AdamW updates and state allocation. |
Review details
Suppressed comments (2)
extension/training/optimizer/adamw.cpp:166
- Valid zero-element tensors are supported by ExecuTorch, but this treats
malloc(0)returningnullptras an allocation failure. On such allocators an empty parameter/gradient incorrectly returnsMemoryAllocationFailed; only treat null as failure for nonzero storage and skip zero-length initialization.
if (m_buf_ptr == nullptr || v_buf_ptr == nullptr) {
free(m_buf_ptr);
free(v_buf_ptr);
return Error::MemoryAllocationFailed;
extension/training/optimizer/adamw.cpp:153
- Weight decay is applied before the lazy state allocation below. If either
mallocfails,step()returnsMemoryAllocationFailedafter mutatingpbut before updating its moments; retrying decays it again and can leave a partially applied step. Allocate and initialize the state before mutating parameters, or preflight all allocations for the step.
if (weight_decay != 0.0) {
add_out_hack(p, p, -lr * weight_decay, p);
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| auto g = named_gradient->second; | ||
| auto p = param_iter->second; |
da0ebfa to
a6f3935
Compare
|
Dispositioning the three Copilot findings. All three were also raised on #18848; the
Constant-gradient tests — fair, and fixed. A constant ±1 gradient makes Adam's normalized update very nearly sign-only, so the existing three tests would have passed with a broken moment recurrence or bias correction. Added dtype/layout/shape validation (flagged High) — not fixed here, deliberately. The report is technically correct: Content is byte-identical to #18848 (tree |
There was a problem hiding this comment.
🟡 Changes recommended
Allocation failure can mutate parameters before state initialization, causing repeated weight decay on retry.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
extension/training/optimizer/adamw.cpp:166
g.nbytes()may be zero for a valid empty tensor, andmalloc(0)is allowed to returnnullptr. This condition then returnsMemoryAllocationFailed(and the following zero-lengthmemsetcalls still receive null pointers), so AdamW cannot no-op on empty parameters. Treat zero-byte state as valid and only allocate/initialize when the byte count is nonzero.
if (m_buf_ptr == nullptr || v_buf_ptr == nullptr) {
free(m_buf_ptr);
free(v_buf_ptr);
return Error::MemoryAllocationFailed;
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Ports the existing SGD optimizer at extension/training/optimizer/sgd.{h,cpp}
to AdamW. The algorithmic difference is decoupled weight decay: the
parameter is decayed directly rather than mixing the decay into the
gradient (Loshchilov & Hutter, 2019). Matches torch.optim.AdamW at its
default settings.
C++ only. Python bindings are deliberately left out: the pybindings file
has a standing TODO to build a generic optimizer interface first, so
copying PySGD to PyAdamW now would only add duplication. amsgrad and
maximize are also left out.
The implementation originates from Bryan (github.com/BryanBradfo) in
pull request pytorch#18848, which could not be merged because the contributor
did not complete the Linux Foundation CLA. Credit for the design and
implementation belongs to them; see the pull request description for the
full provenance note.
Fixes pytorch#18766
a6f3935 to
9d1ccdc
Compare
|
Copilot's follow-up on the allocation-failure path was right, and it was on code I added rather than @BryanBradfo's. My Fixed by moving the state lookup and allocation above the decay, so nothing mutates Same commit is on #18848; trees still identical at |
There was a problem hiding this comment.
🟡 Changes recommended
Zero-element tensors can incorrectly trigger MemoryAllocationFailed during state allocation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
| void* m_buf_ptr = malloc(g.nbytes()); | ||
| void* v_buf_ptr = malloc(g.nbytes()); | ||
| if (m_buf_ptr == nullptr || v_buf_ptr == nullptr) { | ||
| free(m_buf_ptr); | ||
| free(v_buf_ptr); | ||
| return Error::MemoryAllocationFailed; | ||
| } | ||
| std::memset(m_buf_ptr, 0, g.nbytes()); | ||
| std::memset(v_buf_ptr, 0, g.nbytes()); |
Upgraded verification: the real C++ now runs, not just a transcriptionEarlier I could only say I had transcribed The two varying-gradient values are the ones taken from Per-parameter state isolation, which the cases above do not cover because they use one optimizer That last line also covers the What this still does not coverThe gtest harness itself ( |
Fixes #18766
Summary
Adds AdamW to the training optimizer extension. It is a port of the existing SGD
implementation at
extension/training/optimizer/sgd.{h,cpp}, with the main algorithmicdifference being decoupled weight decay — the parameter is decayed directly instead of
mixing the decay into the gradient. Matches
torch.optim.AdamWwith default settings.Scope
C++ only. Python bindings are left out on purpose: the pybindings file has a TODO to build
a generic optimizer interface first, so copying
PySGDtoPyAdamWnow would just addduplication.
amsgradandmaximizeare also left out — both rarely used and easy to addlater.
Changes on top of #18848
Two things, both mine:
make_tensor_ptrgained aDeviceparameter after Add AdamW optimizer to extension/training #18848 was written, sothe two state-buffer calls were passing
TensorShapeDynamism::STATICwhere aDeviceisexpected and the deleter where the dynamism is expected. This is what broke every build
job on that PR. Both moment buffers come from
malloc, so they get an explicit CPU device.mallocnull check beforememset, as flagged in review on Add AdamW optimizer to extension/training #18848. The twoallocations were used unconditionally, so a failure on a memory-constrained target
dereferenced null instead of returning
Error::MemoryAllocationFailed.The other review points on #18848 (dtype/shape validation, hyperparameter range checks) are
deliberately not addressed here. @BryanBradfo's argument was that SGD has neither and adding
them to AdamW alone would be inconsistent, and @JacobSzwejbka noted the Copilot review is
noisy. Both optimizers can be hardened together in a follow-up.
Verification
lintrunneris clean, andadamw.cppcompiles against currentmain— I reproduced theexact CI error from #18848 locally and confirmed this clears it.
I could not execute
adamw_test(this machine SIGKILLs freshly built binaries), so insteadI transcribed
AdamW::step's arithmetic verbatim, including the float round-trips, anddiffed it against
torch.optim.AdamWover six configurations:The decoupled-weight-decay ordering matches and all four gtest expectations agree with
PyTorch independently. @BryanBradfo reported
buck2 test //extension/training/optimizer/test:adamw_testpassing 6/6 and the SGD regression staying green on #18848; someone with a working Buck
setup should confirm that still holds, since the tests are a Buck-only target and CI does
not run them.
cc @JacobSzwejbka
The build fix and this PR were prepared with Claude Code.