Skip to content

[autograd] Extract Node class into node.h#179765

Closed
colesbury wants to merge 6 commits intogh/colesbury/4/basefrom
gh/colesbury/4/head
Closed

[autograd] Extract Node class into node.h#179765
colesbury wants to merge 6 commits intogh/colesbury/4/basefrom
gh/colesbury/4/head

Conversation

@colesbury
Copy link
Copy Markdown
Member

@colesbury colesbury commented Apr 8, 2026

Stack from ghstack (oldest at bottom):

Move the Node class definition from function.h into a new node.h
header. function.h becomes a thin wrapper that includes node.h and
provides the free functions (create_gradient_edge, collect_next_edges,
etc.) that depend on variable.h.

This separation is needed to break the include cycle between
function.h and variable.h: function.h includes variable.h, and
variable.h needs the complete Node type for upcoming intrusive_ptr
conversion. node.h avoids this cycle by not including variable.h or
graph_task.h.

Authored with Claude.

[ghstack-poisoned]
@pytorch-bot
Copy link
Copy Markdown

pytorch-bot Bot commented Apr 8, 2026

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/179765

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit c5e8a54 with merge base afb9b15 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot

This comment was marked as resolved.

[ghstack-poisoned]
[ghstack-poisoned]
colesbury added a commit to colesbury/pytorch that referenced this pull request Apr 9, 2026
Move the Node class definition from function.h into a new node.h
header. function.h becomes a thin wrapper that includes node.h and
provides the free functions (create_gradient_edge, collect_next_edges,
etc.) that depend on variable.h.

This separation is needed to break the include cycle between
function.h and variable.h: function.h includes variable.h, and
variable.h needs the complete Node type for upcoming intrusive_ptr
conversion. node.h avoids this cycle by not including variable.h or
graph_task.h.

Authored with Claude.

ghstack-source-id: e040d8b
Pull-Request: pytorch#179765
colesbury added a commit to colesbury/pytorch that referenced this pull request Apr 10, 2026
Move the Node class definition from function.h into a new node.h
header. function.h becomes a thin wrapper that includes node.h and
provides the free functions (create_gradient_edge, collect_next_edges,
etc.) that depend on variable.h.

This separation is needed to break the include cycle between
function.h and variable.h: function.h includes variable.h, and
variable.h needs the complete Node type for upcoming intrusive_ptr
conversion. node.h avoids this cycle by not including variable.h or
graph_task.h.

Authored with Claude.

ghstack-source-id: e040d8b
Pull-Request: pytorch#179765
[ghstack-poisoned]
uint32_t input_nr = input_metadata_.size();
auto meta_shape = MetadataShape{std::in_place_type<SymIntSmallVec>, shape};
input_metadata_.emplace_back(
options, meta_shape, is_tensor_subclass, is_nested, grad_dtype);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should meta_shape be std::moved here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The motivation for doing this extraction in its own commit is to keep the straightforward refactoring separate from other changes. I don't want to mix in unrelated improvements in this commit.

return input_nr;
}

uint32_t add_input_metadata(const at::Tensor& t) noexcept {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should be moved from value instead of const ref. :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think const ref is appropriate. We are emplacing a InputMetadata constructed from the Tensor, not the Tensor itself.

[ghstack-poisoned]
Copy link
Copy Markdown
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

SGTM !

@colesbury colesbury added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 16, 2026
@colesbury
Copy link
Copy Markdown
Member Author

@pytorchbot merge

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch cherry-pick -x afa73b4a0324e6000ff38dd2c661fe87ab2458aa returned non-zero exit code 1

Auto-merging torch/csrc/autograd/function.cpp
CONFLICT (content): Merge conflict in torch/csrc/autograd/function.cpp
Auto-merging torch/csrc/autograd/function.h
CONFLICT (content): Merge conflict in torch/csrc/autograd/function.h
error: could not apply afa73b4a032... [autograd] Extract Node class into node.h
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Details for Dev Infra team Raised by workflow job

[ghstack-poisoned]
@colesbury
Copy link
Copy Markdown
Member Author

@pytorchbot merge

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

pytorchmergebot pushed a commit that referenced this pull request Apr 21, 2026
Stack from [ghstack](https://github.com/ezyang/ghstack/tree/0.14.0) (oldest at bottom):

* #179767
* -> #179766
* #179765
* #179764

Replace std::shared_ptr<Node> with c10::intrusive_ptr<Node> throughout
the autograd system. Node now inherits from c10::intrusive_ptr_target
instead of std::enable_shared_from_this<Node>.

Authored with Claude.

Pull Request resolved: #179766
Approved by: https://github.com/albanD
ghstack dependencies: #179764, #179765
colesbury added a commit that referenced this pull request Apr 22, 2026
Stack from [ghstack](https://github.com/ezyang/ghstack/tree/0.14.0) (oldest at bottom):

* #179767
* -> #179766
* #179765
* #179764

Replace std::shared_ptr<Node> with c10::intrusive_ptr<Node> throughout
the autograd system. Node now inherits from c10::intrusive_ptr_target
instead of std::enable_shared_from_this<Node>.

Authored with Claude.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx kadeng chauhang amjames Lucaskabela jataylo azahed98 xmfan aditvenk

[ghstack-poisoned]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants