Make LazyGraphExecutor extensible #87218

antoniojkim · 2022-10-18T17:18:11Z

Add LazyGraphExecutor to backend interface so that its is extensible by a vendor backend.

I've made some preliminary methods virtual. Not sure if we want to make all methods in LazyGraphExecutor virtual.

pytorch-bot · 2022-10-18T17:18:13Z

🔗 Helpful Links

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

📄 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 our office hours

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

✅ No Failures, 1 Pending

As of commit 11d1021:
💚 Looks good so far! There are no failures yet. 💚

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

JackCaoG · 2022-10-18T17:37:24Z

@alanwaketan Does this align with what you plan to implement?

torch/csrc/lazy/core/lazy_graph_executor.cpp

alanwaketan

Overall LGTM.

alanwaketan · 2022-10-18T17:43:58Z

torch/csrc/lazy/core/lazy_graph_executor.cpp

 }

+LazyGraphExecutor::~LazyGraphExecutor() = default;


Is there any reasons why this isn't in the header?

alanwaketan · 2022-10-18T17:47:12Z

I guess we don't necessarily need to make all methods virtual all in once. We can just make them on-demand.

alanwaketan · 2022-10-18T17:48:51Z

torch/csrc/lazy/backend/backend_interface.h

@@ -41,7 +42,7 @@ class TORCH_API BackendImplInterface {

  virtual const IrBuilder* GetIrBuilder() const = 0;

-  virtual bool ShouldSyncTensor(const LazyTensorPtr tensor) const;
+  virtual LazyGraphExecutor* GetLazyGraphExecutor() const;


Since this is an API now, maybe a few words to describe it would be great.

Also, have you compare this approach with providing a registration method directly in the LazyGraphExecutor class?

Also, have you compare this approach with providing a registration method directly in the LazyGraphExecutor class?

I have not. Do we anticipate the additional layer of indirection to have much impact on performance?

I don't think performance is the main concern. Since the LazyGraphExecutor becomes a backend interface anyway, it just doesn't feel necessary to have the registration part in the BackendInterface to make the design cleaner.

it just doesn't feel necessary to have the registration part in the BackendInterface to make the design cleaner

I'm not sure what you mean by this. When you say registration, are you referring the to static object initialization?

I'm talking about things similar to BackendRegistrar.

ah, I see now what you mean. I can implement that for LazyGraphExecutor

alanwaketan · 2022-10-18T17:54:03Z

torch/csrc/lazy/core/lazy_graph_executor.h

-  void UnregisterTensor(LazyTensor::Data* data);
+  virtual ~LazyGraphExecutor();
+
+  virtual void RegisterTensor(std::shared_ptr<LazyTensor::Data> data);


Since these become APIs now, maybe provide a few words explaining why people want to override these methods.

wconstab

LGTM- thanks for doing this @antoniojkim and for the feedback @alanwaketan! I like the idea of having the registrar in LazyGraphExecutor, to avoid piling stuff into backend interface.

antoniojkim · 2022-10-19T15:10:55Z

@JackCaoG I don't know if PyTorch/XLA is using the lazy graph executor yet, but if so, this PR will break PyTorch/XLA. If not, please ignore.

JackCaoG · 2022-10-19T17:29:33Z

yea.. I saw RuntimeError: Lazy graph executor not registered. in the CI, @alanwaketan can you arrange a fix?

alanwaketan · 2022-10-19T18:00:44Z

Working on a companion pull request now. @antoniojkim Here is the README to how to land such patches together if you haven't seen it before. Basically, we need the PyTorch PR to update the xla pin pointing to the companion PR and then land the PR once the CI is all green.

alanwaketan · 2022-10-19T19:17:51Z

Here is the companion PR: pytorch/xla#4106. You will need to update https://github.com/pytorch/pytorch/blob/master/.github/ci_commit_pins/xla.txt#L1 with eff277e81fcfdeccba71e75ff40b6e2f3e29e27b.

antoniojkim · 2022-10-19T19:58:07Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-19T20:00:31Z

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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 · 2022-10-19T21:36:19Z

Merge failed

Reason: 2 additional jobs have failed, first few of them are: trunk ,trunk / linux-bionic-cuda11.7-py3.10-gcc7 / test (default, 1, 4, linux.4xlarge.nvidia.gpu)

Details for Dev Infra team

Raised by workflow job

antoniojkim · 2022-10-20T15:49:23Z

@pytorchbot merge

pytorchmergebot · 2022-10-20T15:50:50Z

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 · 2022-10-20T17:16:38Z

Merge failed

Reason: The following mandatory check(s) failed (Rule superuser):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

antoniojkim · 2022-10-21T02:12:25Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-21T02:15:55Z

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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 · 2022-10-21T02:31:12Z

Merge failed

Reason: HTTP Error 502: Bad Gateway

Details for Dev Infra team

Raised by workflow job

antoniojkim · 2022-10-21T14:26:42Z

@pytorchbot merge

pytorchmergebot · 2022-10-21T14:28:10Z

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

github-actions · 2022-10-21T14:29:42Z

Hey @antoniojkim.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Add `LazyGraphExecutor` to backend interface so that its is extensible by a vendor backend. I've made some preliminary methods virtual. Not sure if we want to make all methods in `LazyGraphExecutor` virtual. Pull Request resolved: pytorch#87218 Approved by: https://github.com/wconstab, https://github.com/alanwaketan

antoniojkim requested review from Krovatkin, desertfire, JackCaoG and wconstab October 18, 2022 17:18

pytorchbot added the open source label Oct 18, 2022

alanwaketan reviewed Oct 18, 2022

View reviewed changes

torch/csrc/lazy/core/lazy_graph_executor.cpp Show resolved Hide resolved

alanwaketan reviewed Oct 18, 2022

View reviewed changes

wconstab approved these changes Oct 19, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 19, 2022

antoniojkim force-pushed the antoniojkim/extensible_lazy_graph_executor branch from ef4cc8c to fa0d4ab Compare October 19, 2022 14:34

antoniojkim mentioned this pull request Oct 19, 2022

Reference lazy graph executor llvm/torch-mlir#1507

Merged

alanwaketan mentioned this pull request Oct 19, 2022

[LTC] Adopt the extensible LazyGraphExecutor pytorch/xla#4106

Merged

alanwaketan approved these changes Oct 19, 2022

View reviewed changes

antoniojkim requested a review from a team as a code owner October 19, 2022 19:26

antoniojkim force-pushed the antoniojkim/extensible_lazy_graph_executor branch 2 times, most recently from e1cf63a to 08cbf54 Compare October 20, 2022 15:37

antoniojkim force-pushed the antoniojkim/extensible_lazy_graph_executor branch from 08cbf54 to 2946121 Compare October 20, 2022 18:17

antoniojkim added 3 commits October 20, 2022 19:11

Make LazyGraphExecutor extensible

24a4938

Address PR comments

1cec327

Update xla commit

11d1021

antoniojkim force-pushed the antoniojkim/extensible_lazy_graph_executor branch from 2946121 to 11d1021 Compare October 21, 2022 02:11

pytorchmergebot added the Merged label Oct 21, 2022

pytorchmergebot closed this in d37dc6f Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make LazyGraphExecutor extensible #87218

Make LazyGraphExecutor extensible #87218

antoniojkim commented Oct 18, 2022

pytorch-bot bot commented Oct 18, 2022 •

edited

JackCaoG commented Oct 18, 2022

alanwaketan left a comment

alanwaketan Oct 18, 2022

alanwaketan commented Oct 18, 2022

alanwaketan Oct 18, 2022

alanwaketan Oct 18, 2022

antoniojkim Oct 18, 2022

alanwaketan Oct 18, 2022

antoniojkim Oct 18, 2022

alanwaketan Oct 18, 2022

antoniojkim Oct 18, 2022

alanwaketan Oct 18, 2022

wconstab left a comment

antoniojkim commented Oct 19, 2022

JackCaoG commented Oct 19, 2022

alanwaketan commented Oct 19, 2022 •

edited

alanwaketan commented Oct 19, 2022 •

edited

antoniojkim commented Oct 19, 2022

pytorchmergebot commented Oct 19, 2022

pytorchmergebot commented Oct 19, 2022

antoniojkim commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

antoniojkim commented Oct 21, 2022

pytorchmergebot commented Oct 21, 2022

pytorchmergebot commented Oct 21, 2022

antoniojkim commented Oct 21, 2022

pytorchmergebot commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

		}

		LazyGraphExecutor::~LazyGraphExecutor() = default;

Make LazyGraphExecutor extensible #87218

Make LazyGraphExecutor extensible #87218

Conversation

antoniojkim commented Oct 18, 2022

pytorch-bot bot commented Oct 18, 2022 • edited

🔗 Helpful Links

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

✅ No Failures, 1 Pending

JackCaoG commented Oct 18, 2022

alanwaketan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwaketan commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wconstab left a comment

Choose a reason for hiding this comment

antoniojkim commented Oct 19, 2022

JackCaoG commented Oct 19, 2022

alanwaketan commented Oct 19, 2022 • edited

alanwaketan commented Oct 19, 2022 • edited

antoniojkim commented Oct 19, 2022

pytorchmergebot commented Oct 19, 2022

Merge started

pytorchmergebot commented Oct 19, 2022

Merge failed

antoniojkim commented Oct 20, 2022

pytorchmergebot commented Oct 20, 2022

Merge started

pytorchmergebot commented Oct 20, 2022

Merge failed

antoniojkim commented Oct 21, 2022

pytorchmergebot commented Oct 21, 2022

Merge started

pytorchmergebot commented Oct 21, 2022

Merge failed

antoniojkim commented Oct 21, 2022

pytorchmergebot commented Oct 21, 2022

Merge started

github-actions bot commented Oct 21, 2022

pytorch-bot bot commented Oct 18, 2022 •

edited

alanwaketan commented Oct 19, 2022 •

edited

alanwaketan commented Oct 19, 2022 •

edited