Misc changes from compiled autograd branch #104316

jansel · 2023-06-28T01:48:37Z

Stack from ghstack (oldest at bottom):

This PR pulls out some standalone changes from #103822

cc @voznesenskym @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @ipiszy @chenyang78

[ghstack-poisoned]

pytorch-bot · 2023-06-28T01:48:41Z

🔗 Helpful Links

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

📄 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.

✅ No Failures

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

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

wconstab

these lgtm, except maybe the private thing. maybe @albanD can also take a look

wconstab · 2023-06-28T03:36:18Z

torch/fx/experimental/symbolic_shapes.py

+            self.create_symbol(
+                value,
+                source=source,
+                dynamic_dim=dynamic_dim,


was it significant to remove constraint_dim=none here?

was there any actual change other than code motion?

It is just code motion so it can be reused in the next PR.

constraint_dim=none is the default value.

wconstab · 2023-06-28T03:37:46Z

torch/_functorch/aot_autograd.py

@@ -2807,12 +2807,6 @@ def forward(ctx, *deduped_flat_tensor_args):
            )

            num_outputs = CompiledFunction.metadata.num_outputs
-            num_outputs_aliased_to_inputs = (


dead code removal?

wconstab · 2023-06-28T03:38:29Z

tools/autograd/templates/Functions.h

@@ -52,7 +52,6 @@ struct TypeAndSize {

  Tensor zeros() { return at::zeros_symint(sym_sizes, options); }

-private:


is it nontrivial to utilize accessors for these from the codegen code or just more convenient this way?

The visitors need to mutate these. I could do a mutable reference accessor, but this is more convenient.

torch/_dynamo/variables/builder.py

albanD · 2023-06-28T15:54:49Z

aten/src/ATen/TensorGeometry.h

+  c10::SymInt& mutable_storage_offset() {
+    return storage_offset_;
+  }
+  void recompute() {


Should this be called from the constructor as well?

It seems like the constructor is using a cached copy of numel?

Yes and we compute it alongside of the stride for the contiguous case.
I guess not re-use it is ok then!

aten/src/ATen/TensorGeometry.h

albanD · 2023-06-28T18:17:01Z

aten/src/ATen/TensorGeometry.h

@@ -113,6 +113,27 @@ struct TORCH_API TensorGeometry {
    return r;
  }

+  std::vector<c10::SymInt>& mutable_sizes() {


What's the thread-safety story on these?
I haven't dived into how they are used in the other PR but this structure is saved on the graph which can be used in multiple threads at the same time. So if this is not read-only anymore, do we need to add locking?

The code using this in the other PR only uses a single thread.

How do you guarantee that? The user could be calling backward() from multiple threads?

We could add a lock around compiled_autograd tracing. Since we are mutating nodes in-place, it is not thread safe.

Actually, this isn't needed. This is already protected by:

pytorch/torch/csrc/dynamo/compiled_autograd_py.cpp

Line 285 in 548ad77

pybind11::gil_scoped_acquire gil;

The gil doesn't create critical blocks. In can be released in most places in there and another thread pick up.
Also you could have a vanilla backward running in another thread that would not hold the gil right?

I added a lock in the other PR. compiled_autograd is enabled per-process, so mixing it between threads is not supported.

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

Skylion007

Nit

aten/src/ATen/TensorGeometry.h

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

jansel · 2023-07-06T17:23:38Z

@pytorchbot merge

pytorchmergebot · 2023-07-06T17:25:29Z

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 · 2023-07-06T19:01:25Z

Merge failed

Reason: 1 jobs have failed, first few of them are: inductor / linux-focal-cpu-py3.8-gcc7-inductor / test (inductor_torchbench_dynamic_cpu_accuracy, 1, 1, linux.12xlarge)

Details for Dev Infra team

Raised by workflow job

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

jansel · 2023-07-08T20:57:31Z

@pytorchbot merge

pytorchmergebot · 2023-07-08T20:59:12Z

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

Misc changes from compiled autograd branch

fdb377d

[ghstack-poisoned]

jansel requested review from albanD and soulitzer as code owners June 28, 2023 01:48

pytorch-bot bot added the release notes: AO frontend label Jun 28, 2023

jansel mentioned this pull request Jun 28, 2023

Compiled autograd #103822

Closed

github-actions bot added ciflow/inductor module: dynamo labels Jun 28, 2023

jansel requested review from ezyang and voznesenskym June 28, 2023 01:50

wconstab reviewed Jun 28, 2023

View reviewed changes

albanD reviewed Jun 28, 2023

View reviewed changes

Update on "Misc changes from compiled autograd branch"

4ec96b2

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

jansel requested a review from albanD June 29, 2023 16:26

Skylion007 reviewed Jun 29, 2023

View reviewed changes

aten/src/ATen/TensorGeometry.h Outdated Show resolved Hide resolved

Update on "Misc changes from compiled autograd branch"

9b5e8e9

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

ezyang approved these changes Jul 5, 2023

View reviewed changes

Update on "Misc changes from compiled autograd branch"

b10555d

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 6, 2023

pytorchmergebot added the merging label Jul 6, 2023

pytorchmergebot removed the merging label Jul 6, 2023

jansel added 3 commits July 6, 2023 16:11

Update on "Misc changes from compiled autograd branch"

5cdd0e9

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

Update on "Misc changes from compiled autograd branch"

05bac9d

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

Update on "Misc changes from compiled autograd branch"

b9276f5

This PR pulls out some standalone changes from #103822 cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx ipiszy chenyang78 [ghstack-poisoned]

pytorchmergebot added the merging label Jul 8, 2023

pytorchmergebot added Merged and removed merging labels Jul 8, 2023

pytorchmergebot closed this in dffcf99 Jul 8, 2023

facebook-github-bot deleted the gh/jansel/138/head branch July 12, 2023 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc changes from compiled autograd branch #104316

Misc changes from compiled autograd branch #104316

jansel commented Jun 28, 2023 •

edited

pytorch-bot bot commented Jun 28, 2023 •

edited

wconstab left a comment

wconstab Jun 28, 2023

jansel Jun 28, 2023

wconstab Jun 28, 2023

jansel Jun 28, 2023

wconstab Jun 28, 2023

jansel Jun 28, 2023

albanD Jun 28, 2023

jansel Jun 28, 2023

albanD Jun 28, 2023

albanD Jun 28, 2023

jansel Jun 28, 2023

albanD Jul 5, 2023

jansel Jul 6, 2023 •

edited

jansel Jul 6, 2023

albanD Jul 6, 2023

jansel Jul 6, 2023

Skylion007 left a comment

jansel commented Jul 6, 2023

pytorchmergebot commented Jul 6, 2023

pytorchmergebot commented Jul 6, 2023

jansel commented Jul 8, 2023

pytorchmergebot commented Jul 8, 2023

		@@ -52,7 +52,6 @@ struct TypeAndSize {

		Tensor zeros() { return at::zeros_symint(sym_sizes, options); }

		private:

Misc changes from compiled autograd branch #104316

Misc changes from compiled autograd branch #104316

Conversation

jansel commented Jun 28, 2023 • edited

pytorch-bot bot commented Jun 28, 2023 • edited

🔗 Helpful Links

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

✅ No Failures

wconstab left a 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

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

jansel Jul 6, 2023 • edited

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

Skylion007 left a comment

Choose a reason for hiding this comment

jansel commented Jul 6, 2023

pytorchmergebot commented Jul 6, 2023

Merge started

pytorchmergebot commented Jul 6, 2023

Merge failed

jansel commented Jul 8, 2023

pytorchmergebot commented Jul 8, 2023

Merge started

jansel commented Jun 28, 2023 •

edited

pytorch-bot bot commented Jun 28, 2023 •

edited

jansel Jul 6, 2023 •

edited