Skip to content

Conversation

ysiraichi
Copy link
Collaborator

@ysiraichi ysiraichi commented Aug 21, 2023

Fix: #107187

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Aug 21, 2023

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 278fa5a with merge base b7624fc (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

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

Fix: #107187

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

[ghstack-poisoned]
Comment on lines 395 to 429
# OpInfo that call wrapper_set_seed.
# Ref: https://github.com/pytorch/pytorch/issues/107187
# Since then, manual_seed calls graph-breaks. Thus, in that case, we need to allow
# Python code.
inductor_nopython_error = {
"item",
"cauchy",
"exponential",
"geometric",
"log_normal",
"normal",
"normal.in_place",
"normal.number_mean",
"uniform",
"nn.functional.fractional_max_pool2d",
"nn.functional.fractional_max_pool3d",
"nn.functional.rrelu",
"nn.functional.scaled_dot_product_attention",
"svd_lowrank",
"pca_lowrank",
"randn",
"randn_like",
"rand_like",
"randint",
"randint_like",
"empty_strided",
"multinomial",
"bernoully",
"nn.functional.dropout",
"nn.functional.dropout2d",
"nn.functional.dropout3d",
"nn.functional.alpha_dropout",
"nn.functional.feature_alpha_dropout",
"nn.functional.multi_head_attention_forward",
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are many tests failing because they called torch.manual_seed in their OpInfo. Since we are graph-breaking on manual_seed calls, we need to allow Python code for them.

Copy link
Contributor

@eellison eellison Aug 22, 2023

Choose a reason for hiding this comment

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

Can we instead monkey-patch manual_seed to be a no-op in the tests, and then set manual_seed ourselves outside the test invocation ?

Would be nice to get coverage for these ops. I think that and adding "fallback_random": True to below would cause a lot to pass.

    @torch._inductor.config.patch(
        {"implicit_fallbacks": False, "triton.autotune_pointwise": False}
    )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. Apparently, it worked without fallback_random, but simply making wrapper_set_seed a noop. Am I missing anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

The opfinos for all the rng functions expect failures right now: https://github.com/pytorch/pytorch/blob/main/test/inductor/test_torchinductor_opinfo.py#L281

I think why it worked

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But, take a look at this part of check_model function. We already set torch.manual_seed(0) before calling both the plain model and the optimized one. So, maybe this is not the problem!?

torch.manual_seed(0)
correct = model(*ref_inputs, **ref_kwargs)
# downcast the model back if needed
if reference_in_float and has_lowp_args:
if hasattr(model, "to"):
model = model.to(original_lowp_dtype)
torch._inductor.metrics.reset()
called = False
def compile_fx_wrapper(model_, example_inputs_):
nonlocal called
called = True
return compile_fx(model_, example_inputs_)
def run(*ex, **kwargs):
return model(*ex, **kwargs)
run = torch._dynamo.optimize(compile_fx_wrapper, nopython=nopython)(run)
torch.manual_seed(0)
actual = run(*example_inputs, **kwargs)

For randint and some others (guess), I think they have different pseudo-generating algorithms:

Plus, as far as I've seen, randint doesn't seem to get its seed from the default_generator (which is the one affected by manual_seed).

Fix: #107187

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov anijain2305

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Aug 21, 2023
Fix: #107187

ghstack-source-id: ef61dd5
Pull Request resolved: #107594
Fix: #107187

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov anijain2305

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Aug 22, 2023
Fix: #107187

ghstack-source-id: bf1921e
Pull Request resolved: #107594
Fix: #107187

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov anijain2305

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Aug 22, 2023
Fix: #107187

ghstack-source-id: 3596033
Pull Request resolved: #107594
Fix: #107187

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov anijain2305

[ghstack-poisoned]
Fix: #107187

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov anijain2305

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Aug 25, 2023
Fix: #107187

ghstack-source-id: d76a72e
Pull Request resolved: #107594
@ysiraichi ysiraichi requested a review from voznesenskym August 28, 2023 15:01
@eellison eellison added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 28, 2023


torch.testing._internal.common_methods_invocations.wrapper_set_seed = wrapper_set_seed
torch.testing._internal.common_methods_invocations.wrapper_set_seed = (
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be patching this inside of the test, with unittest.patch, so we dont inadvertently affect other test and maybe be calling freeze_rng_state as well

@ysiraichi
Copy link
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
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

@facebook-github-bot facebook-github-bot deleted the gh/ysiraichi/76/head branch September 3, 2023 14:24
@huydhn
Copy link
Contributor

huydhn commented Sep 4, 2023

@pytorchbot revert -m 'Sorry for reverting your change, but it has an import issue that breaks internal code' -c ghfirst

Here is the import error from the diff D48890949:

======================================================================
ERROR: test_intersect_ray_with_plane_behind_sphere (arvr.projects.eye_tracking.Fringe.test.test_geometry.TestStructuredLightGeometry)
Test intersection with a plane behind a sphere.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/par_unpack.mGtwZSdDV/arvr/projects/eye_tracking/Fringe/test/test_geometry.py", line 23, in setUp
    torch.random.manual_seed(random_seed)
  File "/tmp/par_unpack.mGtwZSdDV/torch/_compile.py", line 22, in inner
    import torch._dynamo
  File "/tmp/par_unpack.mGtwZSdDV/torch/_dynamo/__init__.py", line 2, in <module>
    from . import allowed_functions, convert_frame, eval_frame, resume_execution
  File "/tmp/par_unpack.mGtwZSdDV/torch/_dynamo/convert_frame.py", line 23, in <module>
    from . import config, exc
  File "/tmp/par_unpack.mGtwZSdDV/torch/_dynamo/exc.py", line 15, in <module>
    from torch.fb.exportdb.logging import exportdb_error_message
ModuleNotFoundError: No module named 'torch.fb'
----------------------------------------------------------------------

The internal module torch.fb was pulled via from ._compile import _disable_dynamo. cc @eellison As you have access to the internal diff, could you please help @ysiraichi fix the internal import error and reland the change?

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@ysiraichi your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Sep 4, 2023
This reverts commit 6ad5568.

Reverted #107594 on behalf of https://github.com/huydhn due to Sorry for reverting your change, but it has an import issue that breaks internal code ([comment](#107594 (comment)))
@ysiraichi ysiraichi restored the gh/ysiraichi/76/head branch September 5, 2023 20:52
@ysiraichi ysiraichi deleted the gh/ysiraichi/76/head branch September 5, 2023 20:58
@ysiraichi ysiraichi restored the gh/ysiraichi/76/head branch September 5, 2023 20:58
@ysiraichi ysiraichi deleted the gh/ysiraichi/76/head branch September 5, 2023 20:58
@ysiraichi ysiraichi restored the gh/ysiraichi/76/head branch September 5, 2023 20:58
@eellison
Copy link
Contributor

eellison commented Sep 5, 2023

@ysiraichi would you try adding changing this to the following and resubmitting? :

if is_fbcode():
    try:
        from torch.fb.exportdb.logging import exportdb_error_message
    except ModuleNotFoundError:

        def exportdb_error_message(case_name):
            return ""

@facebook-github-bot facebook-github-bot deleted the gh/ysiraichi/76/head branch September 6, 2023 14:22
pytorchmergebot pushed a commit that referenced this pull request Sep 7, 2023
ysiraichi added a commit that referenced this pull request Sep 12, 2023
Re-landing: #108647 (old #107594)

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 12, 2023
Re-landing: #108647 (old #107594)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 12, 2023
Re-landing: #108647 (old #107594)

ghstack-source-id: 8367e16
Pull Request resolved: #109109
ysiraichi added a commit that referenced this pull request Sep 14, 2023
Re-landing: #108647 (old #107594)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 14, 2023
Re-landing: #108647 (old #107594)

ghstack-source-id: b5dd246
Pull Request resolved: #109109
ysiraichi added a commit that referenced this pull request Sep 22, 2023
Re-landing: #108647 (old #107594)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 22, 2023
Re-landing: #108647 (old #107594)

ghstack-source-id: 2292bf0
Pull Request resolved: #109109
ysiraichi added a commit that referenced this pull request Sep 23, 2023
Re-landing: #108647 (old #107594)

ghstack-source-id: 26f3d4b
Pull Request resolved: #109109
ysiraichi added a commit that referenced this pull request Sep 23, 2023
Re-landing: #108647 (old #107594)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 23, 2023
Re-landing: #108647 (old #107594)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 26, 2023
Re-landing: #108647 (old #107594)

ghstack-source-id: 993e62b
Pull Request resolved: #109109
ysiraichi added a commit that referenced this pull request Sep 26, 2023
Re-landing: #108647 (old #107594)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
ysiraichi added a commit that referenced this pull request Sep 26, 2023
Re-landing: #108647 (old #107594)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 ipiszy ngimel yf225 chenyang78 kadeng muchulee8 aakhundov

[ghstack-poisoned]
pytorchmergebot pushed a commit that referenced this pull request Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants