Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BE] Fix SIM118 in-dict-keys #100339

Closed
wants to merge 5 commits into from

Conversation

Use {key} in {dict} instead of {key} in {dict}.keys()

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Apr 30, 2023

🔗 Helpful Links

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

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

❌ 12 New Failures, 2 Unrelated Failures

As of commit 36598f4:

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following jobs failed but were present on the merge base e779a30:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

justinchuby added a commit that referenced this pull request Apr 30, 2023
Use {key} in {dict} instead of {key} in {dict}.keys()

ghstack-source-id: d6afa1f8deadc56deb609100f84534802a0f3fc7
Pull Request resolved: #100339
@@ -96,7 +96,7 @@ def __init__(self, prof: profile):
self.profile = prof
self.metrics: Dict[EventKey, EventMetrics] = {}
self.compute_self_time()
self.event_keys = sorted((e for e in self.metrics.keys()),
self.event_keys = sorted((e for e in self.metrics),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.event_keys = sorted((e for e in self.metrics),
self.event_keys = sorted(self.metrics,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just opened an issue to add a new flake8 comprehension rule to detect these. :) adamchainz/flake8-comprehensions#503

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 probably is one already IIRC. I am not sure if it is enabled in our config

Copy link
Collaborator

Choose a reason for hiding this comment

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

@justinchuby Nah, I enabled almost every flake8-comprehension rule in our config, even the ones I added recently. :)

Use {key} in {dict} instead of {key} in {dict}.keys()

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
Use {key} in {dict} instead of {key} in {dict}.keys()

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
justinchuby added a commit to justinchuby/pytorch that referenced this pull request Apr 30, 2023
Use {key} in {dict} instead of {key} in {dict}.keys()

ghstack-source-id: a8b1f7c7e412fd43c17ee60e599df8ca96c21fe4
Pull Request resolved: pytorch#100339

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Use {key} in {dict} instead of {key} in {dict}.keys()

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
justinchuby added a commit to justinchuby/pytorch that referenced this pull request Apr 30, 2023
Use {key} in {dict} instead of {key} in {dict}.keys()

ghstack-source-id: a8b1f7c7e412fd43c17ee60e599df8ca96c21fe4
Pull Request resolved: pytorch#100339

Signed-off-by: Justin Chu <justinchu@microsoft.com>
@Skylion007 Skylion007 added better-engineering Relatively self-contained tasks for better engineering contributors topic: not user facing topic category labels Apr 30, 2023
Use {key} in {dict} instead of {key} in {dict}.keys()

cc soumith voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
Copy link
Contributor

Choose a reason for hiding this comment

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

changes in this file LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Also LGTM

@@ -269,7 +269,7 @@ def update_bn(loader, model, device=None):

model(input)

for bn_module in momenta.keys():
for bn_module in momenta:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use an items() call here since we access the key in the next line?

k
for k in dict(module.named_parameters()).keys()
if LINEAR_SKIP in k
k for k in dict(module.named_parameters()) if LINEAR_SKIP in k
Copy link
Collaborator

@Skylion007 Skylion007 May 1, 2023

Choose a reason for hiding this comment

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

the dict() call here is unnecessary now, just unpack the tuple.

Copy link
Contributor

@NivekT NivekT left a comment

Choose a reason for hiding this comment

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

Changes under torch/utils/data/datapipes/iterLGTM!

@albanD
Copy link
Collaborator

albanD commented May 1, 2023

Hey!
I think it would be better to split this kind of PRs into smaller chunks (since you're already using ghstack, that shouldn't be a problem).
That will make reviewing a lot easier and also allow you to pinpoint the actual breakage from CI more easily.

This comment applies to most PRs in this stack.

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Jun 30, 2023
@facebook-github-bot facebook-github-bot deleted the gh/justinchuby/41/head branch August 20, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
better-engineering Relatively self-contained tasks for better engineering contributors ciflow/inductor module: dynamo module: inductor open source release notes: quantization release notes category release notes: releng release notes category Stale topic: not user facing topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants