Purge MINIFIER_SPAWNED by voznesenskym · Pull Request #90089 · pytorch/pytorch

voznesenskym · 2022-12-02T21:54:21Z

Stack from ghstack (oldest at bottom):

Purge MINIFIER_SPAWNED

cc @mlazos @soumith @yanboliang @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @chunyuan-w @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @desertfire

Purge MINIFIER_SPAWNED [ghstack-poisoned]

pytorch-bot · 2022-12-02T21:54:23Z

🔗 Helpful Links

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

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

❌ 1 Failures

As of commit 6bd7b90:

The following jobs have failed:

linux-bionic-py3_7-clang8-xla / test (xla, 1, 1, linux.2xlarge)

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

Purge MINIFIER_SPAWNED ghstack-source-id: 718020a Pull Request resolved: #90089

torch/_dynamo/optimizations/backends.py

ezyang · 2022-12-02T22:11:38Z

torch/_dynamo/debug_utils.py

            )
        )
-        return True
+        raise


We need this as it moved to runtime

OK, well, if you want to change the behavior here, you need to at least update the warning above.

But the original intent of squelching the error still seems valid; if you get stuck on an unrelated exception, you're never going to get to the actual failure that you wanted to minify. So I don't really understand your stated justification...

Purge MINIFIER_SPAWNED cc mlazos soumith yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

Purge MINIFIER_SPAWNED ghstack-source-id: c34b211 Pull Request resolved: #90089 lint

Purge MINIFIER_SPAWNED cc mlazos soumith yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

torch/_dynamo/debug_utils.py

ezyang · 2022-12-03T20:56:19Z

torch/_dynamo/debug_utils.py

-    return gm
+    def backend_accuracy_eval_fwd(*args):
+        # Set the eval mode to remove randomness.
+        gm.eval()


This looks like an unnecessary behavior change; previously gm was changed to eval() mode at compile time, now it is reset to eval time every runtime.

...arguably we shouldn't be changing the model to eval mode at all, doesn't this mean running with accuracy minifier changes the results of your model...

Good point, let's go back to the prior way where we ensure we compile.

ezyang · 2022-12-03T20:58:21Z

torch/_dynamo/debug_utils.py

+            return not same_two_models(gm, compiled_gm, args)
+
+        # (4) Check Accuracy
+        if not same_two_models(gm, compiled_gm, args):


Use module_fails_accuracy?

ezyang · 2022-12-03T21:01:13Z

torch/_dynamo/debug_utils.py

+            minifier(
+                gm,
+                example_inputs,
+                module_fails=module_fails_accuracy,


This looks questionable. The original fails_fn recompiles the passed in gm. However, your fails_fn here simply reuses the original compiled_gm as is. This seems wrong: I'd expect the minifier to be modifying the gm passed in and expecting it to recompile. Without recompiling it I find it hard to see how the minifier could do the correct thing here.

Good point, I bet it minifies but not the same way, as the backend is taken out. Let me revisit this.

ezyang

The new code looks buggy

Purge MINIFIER_SPAWNED cc mlazos soumith yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

Purge MINIFIER_SPAWNED ghstack-source-id: 988c850 Pull Request resolved: #90089 lint Feedback Comments

ezyang · 2022-12-05T16:30:42Z

torch/_dynamo/debug_utils.py


-    return not same_two_models(gm, compiled_gm, example_inputs, only_fwd)
+    if real_args is None:
+        real_args = example_inputs


Is example inputs expected to be non-fake if real args is None?

ezyang · 2022-12-05T16:36:23Z

torch/_dynamo/debug_utils.py

-    return gm
+            # (4) Bind the current compiler_fn and real_args to accuracy_eval
+            # We need to do this in order to ensure that (a) we correctly recompile on every
+            # minification, and (b) that the args captured here, in the runtime, are passed to eval


This comment is backwards. To "correctly recompile on every minification", the bug in https://github.com/pytorch/pytorch/pull/90089/files/ae9f856428538468b5da8825e9d796790b010db6#r1038871303 was that you didn't pass backend_accuracy_fails to the minifier at all. Binding compiler_fn/real_args has nothing to do with ensuring that we actually recompile; you must bind them for the simpler reason that the minifier isn't going to pass those arguments directly lol

Also, it would be good to understand why the minifier tests did not capture this case; maybe there is a simple test that can be added for this bug.

This comment is backwards. To "correctly recompile on every minification", the bug in https://github.com/pytorch/pytorch/pull/90089/files/ae9f856428538468b5da8825e9d796790b010db6#r1038871303 was that you didn't pass backend_accuracy_fails to the minifier at all. Binding compiler_fn/real_args has nothing to do with ensuring that we actually recompile; you must bind them for the simpler reason that the minifier isn't going to pass those arguments directly lol

I was passing a different function, identical to this except for recompilation. So now I'm confused by what you mean.

The comment says "Bind the current compiler_fn and real_args to accuracy_eval; we need to do this". I parse this as "we need to [Bind the current compiler_fn and real_args]". Let's suppose I removed this binding. Then I would expect this code to fail with "not enough parameters passed".

ezyang

I'm still very skeptical about the change in same_two_models. There are also comments I'd like to see addressed.

voznesenskym · 2022-12-06T22:42:18Z

@anijain2305 ping

github-actions · 2023-02-04T23:33:52Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

Purge

9a70167

Purge MINIFIER_SPAWNED [ghstack-poisoned]

voznesenskym mentioned this pull request Dec 2, 2022

Use dynamo fake tensor mode in aot_autograd, move aot_autograd compilation to lowering time [Merger of 89672 and 89773] #90039

Closed

voznesenskym added a commit that referenced this pull request Dec 2, 2022

Purge

b4208a0

Purge MINIFIER_SPAWNED ghstack-source-id: 718020a Pull Request resolved: #90089

github-actions bot added ciflow/inductor module: dynamo labels Dec 2, 2022

github-actions bot requested review from Chillee, SherlockNoMad, albanD, anjali411, antoniojkim, bdhirsh, ezyang, miladm and wconstab December 2, 2022 21:54

voznesenskym changed the title ~~Purge~~ Purge MINIFIER_SPAWNED Dec 2, 2022

ezyang reviewed Dec 2, 2022

View reviewed changes

torch/_dynamo/optimizations/backends.py Outdated Show resolved Hide resolved

ezyang reviewed Dec 2, 2022

View reviewed changes

Update on "Purge MINIFIER_SPAWNED"

edf7d15

Purge MINIFIER_SPAWNED cc mlazos soumith yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

voznesenskym added a commit that referenced this pull request Dec 2, 2022

Purge

ddc887d

Purge MINIFIER_SPAWNED ghstack-source-id: c34b211 Pull Request resolved: #90089 lint

voznesenskym added the topic: not user facing topic category label Dec 3, 2022

Update on "Purge MINIFIER_SPAWNED"

ae9f856

Purge MINIFIER_SPAWNED cc mlazos soumith yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

ezyang reviewed Dec 3, 2022

View reviewed changes

torch/_dynamo/debug_utils.py Show resolved Hide resolved

voznesenskym requested a review from ezyang December 3, 2022 19:19

ezyang reviewed Dec 3, 2022

View reviewed changes

ezyang requested changes Dec 3, 2022

View reviewed changes

Update on "Purge MINIFIER_SPAWNED"

98e84b1

Purge MINIFIER_SPAWNED cc mlazos soumith yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

Update on "Purge MINIFIER_SPAWNED"

6bd7b90

Purge MINIFIER_SPAWNED cc mlazos soumith yanboliang penguinwu anijain2305 EikanWang jgong5 Guobing-Chen chunyuan-w XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx desertfire [ghstack-poisoned]

voznesenskym added a commit that referenced this pull request Dec 4, 2022

Purge

fde8147

Purge MINIFIER_SPAWNED ghstack-source-id: 988c850 Pull Request resolved: #90089 lint Feedback Comments

voznesenskym requested a review from ezyang December 4, 2022 17:48

albanD removed their request for review December 5, 2022 15:30

ezyang reviewed Dec 5, 2022

View reviewed changes

ezyang approved these changes Dec 5, 2022

View reviewed changes

ezyang requested a review from anijain2305 December 5, 2022 16:38

github-actions bot added the Stale label Feb 4, 2023

voznesenskym closed this Feb 11, 2023

facebook-github-bot deleted the gh/voznesenskym/24/head branch June 8, 2023 19:01

Conversation

voznesenskym commented Dec 2, 2022 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Dec 2, 2022 • 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/90089

❌ 1 Failures

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

voznesenskym commented Dec 6, 2022

Uh oh!

github-actions bot commented Feb 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

voznesenskym commented Dec 2, 2022 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Dec 2, 2022 •

edited

Loading