Use generators with all/any in torch/optim #78142

kit1980 · 2022-05-24T00:45:18Z

Generator comprehensions with any/all are less verbose and potentially help to save memory/CPU : https://eklitzke.org/generator-comprehensions-and-using-any-and-all-in-python

To make JIT work with this change, I added code to convert GeneratorExp to ListComp. So the whole PR is basically NoOp for JIT, but potentially memory and speed improvement for eager mode.

facebook-github-bot · 2022-05-24T00:45:24Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/78142
📄 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
↩️ [fb-only] Re-run with SSH instructions

✅ 1 Base Failures

As of commit 0fc1580 (more details on the Dr. CI page):

Expand to see more

✅ None of the CI failures appear to be your fault 💚

1/1 broken upstream at merge base 5a559a5 on Jun 22 from 5:28pm to 10:57pm

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

periodic / ios-12-5-1-arm64 / build on Jun 22 from 5:28pm to 10:57pm (38ee99b - 9cbc692)
- 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

facebook-github-bot · 2022-05-25T03:01:05Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

malfet

Looks good to me, but please explain why test_scriptable is getting removed

malfet · 2022-05-25T21:12:38Z

test/jit/test_parametrization.py

            with parametrize.cached():
                traced_model = torch.jit.trace_module(model, {'forward': x})
-
-    def test_scriptable(self):


Hmm, why are you deleting this test?

As I mentioned in the PR description, the test was bad and had a TODO to actually implement and just tested that UnsupportedNodeError is thrown, and with GeneratorExp support a different error would be thrown.

This test was here on purpose to make sure that trying to script this will raise a nice error message and not crash in an obscure way. I think we want to keep it and make sure the new error message is good.

@albanD any suggestions how to best reconcile this? It was just an accident that GeneratorExp was not supported and the test produced somewhat related-looking UnsupportedNodeError.

In that case, I guess we can add a check if it it scripting and if so, raise a nice error?

In that case, I guess we can add a check if it it scripting and if so, raise a nice error?

Hmm, add a check where exactly?

I'm not sure how torchscript fails here so hard to say.
If register_parametrization does fail, then in that function.
If it fails at runtime, I guess in

pytorch/torch/nn/utils/parametrize.py

Line 257 in efdb419

# Unpack the originals for the first parametrization

which is called at runtime. But script might fail before that?

Or within torchscript if that's possible?

@albanD I've updated the PR, please take another look.

facebook-github-bot · 2022-06-07T00:46:12Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-06-07T00:53:08Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-06-07T00:57:20Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-06-07T02:39:26Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-06-07T03:46:15Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-06-19T06:26:58Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-06-23T01:18:48Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kit1980 · 2022-06-23T04:06:42Z

@pytorchbot rebase

pytorchmergebot · 2022-06-23T04:08:08Z

@pytorchbot successfully started a rebase job. Check the current status here

pytorchmergebot · 2022-06-23T04:08:11Z

Successfully rebased sdym/generator-optim onto refs/remotes/origin/master, please pull locally before adding more changes (for example, via git checkout sdym/generator-optim && git pull --rebase)

facebook-github-bot · 2022-06-23T17:29:21Z

@kit1980 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

albanD

SGTM

kit1980 · 2022-06-24T17:22:26Z

@pytorchbot merge

pytorchmergebot · 2022-06-24T17:23:39Z

@pytorchbot successfully started a merge job. Check the current status here

github-actions · 2022-06-24T17:24:25Z

Hey @kit1980.
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.

Summary: Generator comprehensions with any/all are less verbose and potentially help to save memory/CPU : https://eklitzke.org/generator-comprehensions-and-using-any-and-all-in-python To make JIT work with this change, I added code to convert GeneratorExp to ListComp. So the whole PR is basically NoOp for JIT, but potentially memory and speed improvement for eager mode. Also I removed a test from test/jit/test_parametrization.py. The test was bad and had a TODO to actually implement and just tested that UnsupportedNodeError is thrown, and with GeneratorExp support a different error would be thrown. Pull Request resolved: #78142 Approved by: https://github.com/malfet, https://github.com/albanD Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/de7219e8a7f3b2f8e20162f3b5af338df2e110c1 Reviewed By: malfet, albanD Differential Revision: D36653959 Pulled By: kit1980 fbshipit-source-id: 26b68aa46229f59e21019ec1fd1cfeff81657e58

Apparently #78142 made torch.JIT allow for simple generator expressions which allows us to enable rules that replace unnecessary list comprehensions with generators in any/all. This was originally part of #99280 but I split it off into this PR so that it can be easily reverted should anything break. Pull Request resolved: #99890 Approved by: https://github.com/justinchuby, https://github.com/kit1980, https://github.com/malfet

facebook-github-bot added the cla signed label May 24, 2022

kit1980 added the better-engineering Relatively self-contained tasks for better engineering contributors label May 24, 2022

kit1980 requested a review from a team May 24, 2022 00:47

kit1980 marked this pull request as ready for review May 24, 2022 00:47

kit1980 requested a review from albanD as a code owner May 24, 2022 00:47

kit1980 removed the request for review from a team May 24, 2022 04:27

kit1980 closed this May 24, 2022

kit1980 deleted the sdym/generator-optim branch May 24, 2022 18:15

kit1980 restored the sdym/generator-optim branch May 24, 2022 21:54

kit1980 reopened this May 24, 2022

kit1980 marked this pull request as draft May 24, 2022 22:30

kit1980 force-pushed the sdym/generator-optim branch 2 times, most recently from 95b4b07 to 483b655 Compare May 24, 2022 23:34

kit1980 requested a review from malfet May 25, 2022 02:57

kit1980 marked this pull request as ready for review May 25, 2022 02:58

malfet requested changes May 25, 2022

View reviewed changes

kit1980 requested a review from lezcano as a code owner June 7, 2022 00:40

kit1980 force-pushed the sdym/generator-optim branch from 6d373da to f83a466 Compare June 7, 2022 00:43

kit1980 force-pushed the sdym/generator-optim branch from f83a466 to c48be80 Compare June 7, 2022 00:47

kit1980 force-pushed the sdym/generator-optim branch from c48be80 to 2ede2a6 Compare June 7, 2022 00:56

kit1980 force-pushed the sdym/generator-optim branch from 2ede2a6 to 32cd2bf Compare June 7, 2022 02:37

kit1980 force-pushed the sdym/generator-optim branch from 32cd2bf to 4dec7e2 Compare June 7, 2022 03:45

kit1980 force-pushed the sdym/generator-optim branch from 4dec7e2 to efce3b9 Compare June 7, 2022 04:21

kit1980 force-pushed the sdym/generator-optim branch from fcbdb34 to bfd9ce5 Compare June 19, 2022 04:43

kit1980 added ciflow/trunk Trigger trunk jobs on your pull request ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR labels Jun 19, 2022

malfet approved these changes Jun 21, 2022

View reviewed changes

kit1980 force-pushed the sdym/generator-optim branch from bfd9ce5 to d8b6304 Compare June 23, 2022 01:17

kit1980 marked this pull request as ready for review June 23, 2022 04:05

kit1980 added 4 commits June 23, 2022 04:08

Use generators with all/any in torch/optim

fa6c3b3

Convert GeneratorExp to ListComp in JIT

0eee3eb

Delete broken test.

4a80c08

Better parametrization scripting error message

0fc1580

pytorchmergebot force-pushed the sdym/generator-optim branch from d8b6304 to 0fc1580 Compare June 23, 2022 04:08

albanD approved these changes Jun 24, 2022

View reviewed changes

pytorchmergebot added the Merged label Jun 24, 2022

pytorchmergebot closed this in de7219e Jun 24, 2022

kit1980 mentioned this pull request Apr 22, 2023

Fix flake8 lint errors reported by ruff #99784

Closed

pytorch-bot bot added the release notes: jit release notes category label Apr 22, 2023

Skylion007 mentioned this pull request Apr 24, 2023

[BE] Enable C419 rule for any all shortcircuiting #99890

Closed

github-actions bot deleted the sdym/generator-optim branch February 17, 2024 01:54

Use generators with all/any in torch/optim #78142

Use generators with all/any in torch/optim #78142

Uh oh!

Conversation

kit1980 commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ 1 Base Failures

🚧 1 fixed upstream failure:

Uh oh!

facebook-github-bot commented May 25, 2022

Uh oh!

malfet 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!

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!

facebook-github-bot commented Jun 7, 2022

Uh oh!

facebook-github-bot commented Jun 7, 2022

Uh oh!

facebook-github-bot commented Jun 7, 2022

Uh oh!

facebook-github-bot commented Jun 7, 2022

Uh oh!

facebook-github-bot commented Jun 7, 2022

Uh oh!

facebook-github-bot commented Jun 19, 2022

Uh oh!

facebook-github-bot commented Jun 23, 2022

Uh oh!

kit1980 commented Jun 23, 2022

Uh oh!

pytorchmergebot commented Jun 23, 2022

Uh oh!

pytorchmergebot commented Jun 23, 2022

Uh oh!

facebook-github-bot commented Jun 23, 2022

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

kit1980 commented Jun 24, 2022

Uh oh!

pytorchmergebot commented Jun 24, 2022

Uh oh!

github-actions bot commented Jun 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kit1980 commented May 24, 2022 •

edited

Loading

facebook-github-bot commented May 24, 2022 •

edited

Loading