[dynamo][guards] Support cloning of Guard Manager #140223

anijain2305 · 2024-11-10T07:00:42Z

Stack from ghstack (oldest at bottom):

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames

[ghstack-poisoned]

pytorch-bot · 2024-11-10T07:00:45Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

[DomainsOnly] Jobs fail with GLIBC version not found

✅ No Failures

As of commit 10fb73a with merge base f4ce9ac ():
💚 Looks good so far! There are no failures yet. 💚

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

ghstack-source-id: 06e3d28 Pull Request resolved: #140223

Skylion007 · 2024-11-10T15:51:09Z

torch/csrc/dynamo/guards.cpp


+  // For cloning
+  GuardManager(RootGuardManager* root, std::string source, bool is_dict)
+      : _root(root), _source(source), _is_dict(is_dict) {}


Suggested change

: _root(root), _source(source), _is_dict(is_dict) {}

: _root(root), _source(std::move(source)), _is_dict(is_dict) {}

torch/csrc/dynamo/guards.cpp

Skylion007 · 2024-11-10T15:52:35Z

torch/csrc/dynamo/guards.cpp


+  void add_relational_guard_resetter_without_move(
+      std::shared_ptr<RelationalGuard> relational_guard) {
+    _relational_guard_resetters.emplace_back(relational_guard);


Suggested change

_relational_guard_resetters.emplace_back(relational_guard);

_relational_guard_resetters.emplace_back(std::move(relational_guard));

Surprised this isn't covered by clang-tidy. A lot of things should probably moved by value around here.

Skylion007 · 2024-11-10T15:53:46Z

torch/csrc/dynamo/guards.cpp

+      std::vector<Py_ssize_t> indices)
+      : DictGuardManager(
+            cloned_root,
+            source,


Suggested change

source,

std::move(source),

Skylion007 · 2024-11-10T15:54:04Z

torch/csrc/dynamo/guards.cpp

+            size,
+            _expected_type,
+            is_exact_dict_type,
+            indices) {}


Suggested change

indices) {}

std::move(indices)) {}

Skylion007 · 2024-11-10T15:54:55Z

torch/csrc/dynamo/guards.cpp

+        _size(size),
+        _expected_type(_expected_type),
+        _is_exact_dict_type(is_exact_dict_type),
+        _indices(indices) {}


Suggested change

_indices(indices) {}

_indices(std::move(indices)) {}

torch/csrc/dynamo/guards.cpp

Skylion007

Way to many things should be moved in the newly added code. I went through a few of them, but you should really use clang-tidy to find all the args that need to moved though the calls.

Skylion007 · 2024-11-10T15:57:02Z

torch/csrc/dynamo/guards.cpp

+    RootGuardManager* root,
+    std::shared_ptr<RelationalGuard> guard) {
+  // Don't std::move. Shared the pointers between root and cloned_root.
+  root->add_relational_guard_resetter_without_move(guard);


Suggested change

root->add_relational_guard_resetter_without_move(guard);

root->add_relational_guard_resetter_without_move(std::move(guard));

anijain2305 · 2024-11-10T22:50:04Z

Way to many things should be moved in the newly added code. I went through a few of them, but you should really use clang-tidy to find all the args that need to moved though the calls.

Thanks for pointing out. This PR will be WIP for a few weeks (trying to build up to a new functionality in follow up PRs). I will fix the std::move problems before the final review.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 · 2024-11-20T05:21:18Z

@pytorchbot merge

pytorchmergebot · 2024-11-20T05:23:06Z

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

Pull Request resolved: #140250 Approved by: https://github.com/jansel ghstack dependencies: #140223

Pull Request resolved: #140251 Approved by: https://github.com/jansel ghstack dependencies: #140223, #140250

Pull Request resolved: pytorch#140223 Approved by: https://github.com/jansel

Pull Request resolved: pytorch#140250 Approved by: https://github.com/jansel ghstack dependencies: pytorch#140223

Pull Request resolved: pytorch#140251 Approved by: https://github.com/jansel ghstack dependencies: pytorch#140223, pytorch#140250

[dynamo][guards] Support cloning of Guard Manager

d586092

[ghstack-poisoned]

pytorch-bot bot added ciflow/inductor module: dynamo labels Nov 10, 2024

anijain2305 added a commit that referenced this pull request Nov 10, 2024

[dynamo][guards] Support cloning of Guard Manager

b5e84e8

ghstack-source-id: 06e3d28 Pull Request resolved: #140223

Skylion007 reviewed Nov 10, 2024

View reviewed changes

torch/csrc/dynamo/guards.cpp Show resolved Hide resolved

Skylion007 reviewed Nov 10, 2024

View reviewed changes

torch/csrc/dynamo/guards.cpp Show resolved Hide resolved

Skylion007 reviewed Nov 10, 2024

View reviewed changes

anijain2305 changed the title ~~[dynamo][guards] Support cloning of Guard Manager~~ [wip][dynamo][guards] Support cloning of Guard Manager Nov 10, 2024

Update on "[wip][dynamo][guards] Support cloning of Guard Manager"

ff247e9

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

Update on "[wip][dynamo][guards] Support cloning of Guard Manager"

7af1315

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

This was referenced Nov 11, 2024

[dynamo][guards] Introduce a diff_guard_manager #140250

Closed

[dynamo] skip_guard_eval_unsafe stance for power users #140251

Closed

anijain2305 added 2 commits November 11, 2024 12:17

Update on "[wip][dynamo][guards] Support cloning of Guard Manager"

137585c

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

Update on "[wip][dynamo][guards] Support cloning of Guard Manager"

6b7f04a

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 changed the title ~~[wip][dynamo][guards] Support cloning of Guard Manager~~ [dynamo][guards] Support cloning of Guard Manager Nov 11, 2024

Update on "[dynamo][guards] Support cloning of Guard Manager"

0d369a2

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 added topic: not user facing topic category ciflow/trunk Trigger trunk jobs on your pull request labels Nov 11, 2024

anijain2305 added 2 commits November 11, 2024 14:45

Update on "[dynamo][guards] Support cloning of Guard Manager"

aec4ed7

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

Update on "[dynamo][guards] Support cloning of Guard Manager"

1f9cd30

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 mentioned this pull request Nov 19, 2024

[dynamo] Dont erase the cache line on invalidation #140821

Closed

anijain2305 requested review from Skylion007 and jansel November 19, 2024 03:41

anijain2305 added 3 commits November 18, 2024 23:46

Update on "[dynamo][guards] Support cloning of Guard Manager"

09eb6d3

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

Update on "[dynamo][guards] Support cloning of Guard Manager"

5cc5786

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

Update on "[dynamo][guards] Support cloning of Guard Manager"

10fb73a

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

jansel approved these changes Nov 20, 2024

View reviewed changes

pytorchmergebot added the merging label Nov 20, 2024

pytorchmergebot closed this in a864c42 Nov 20, 2024

pytorchmergebot added Merged and removed merging labels Nov 20, 2024

anijain2305 mentioned this pull request Dec 19, 2024

[dynamo] Support user defined dicts #143548

Closed

github-actions bot deleted the gh/anijain2305/585/head branch December 21, 2024 02:05

	: _root(root), _source(source), _is_dict(is_dict) {}
	: _root(root), _source(std::move(source)), _is_dict(is_dict) {}

	_relational_guard_resetters.emplace_back(relational_guard);
	_relational_guard_resetters.emplace_back(std::move(relational_guard));

	root->add_relational_guard_resetter_without_move(guard);
	root->add_relational_guard_resetter_without_move(std::move(guard));

[dynamo][guards] Support cloning of Guard Manager #140223

[dynamo][guards] Support cloning of Guard Manager #140223

Uh oh!

Conversation

anijain2305 commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❗ 1 Active SEVs

✅ No Failures

Uh oh!

Skylion007 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Skylion007 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

Skylion007 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

Skylion007 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

Skylion007 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

Skylion007 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Skylion007 left a comment

Choose a reason for hiding this comment

Uh oh!

Skylion007 Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

anijain2305 commented Nov 10, 2024

Uh oh!

anijain2305 commented Nov 20, 2024

Uh oh!

pytorchmergebot commented Nov 20, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

anijain2305 commented Nov 10, 2024 •

edited

Loading

pytorch-bot bot commented Nov 10, 2024 •

edited

Loading