Skip to content

Conversation

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Dec 17, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit f0ce4aa with merge base b23f11c (image):
💚 Looks good so far! There are no failures yet. 💚

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

yanboliang added a commit that referenced this pull request Dec 17, 2024
ghstack-source-id: 3d00eff
Pull Request resolved: #143374
@yanboliang yanboliang added the topic: not user facing topic category label Dec 17, 2024
[ghstack-poisoned]
yanboliang added a commit that referenced this pull request Dec 18, 2024
@yanboliang yanboliang changed the title [Dynamo] Correct dict_keys [Dynamo] Add DictKeySetVariable to capture dict_keys passed outside of compiled region Dec 18, 2024
self.get_source().make_guard(GuardBuilder.SEQUENCE_LENGTH),
self.get_source().make_guard(GuardBuilder.EQUALS_MATCH),
)
return DictKeySetVariable(items, source=self.source)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that there is another DictKeys variable for tracking dict_keys, but it only derives from the dictionary itself. Within the compiled region, when users call dict.keys(), it essentially wraps the entire dictionary object. However, if dict_keys is passed from outside the compiled region, we can only access the mappingproxy via dict.keys().mapping.

I don’t think it’s necessary to add another DICT_KEYS guards for the mappingproxy again because:
1. If dict_keys is used together with the dictionary object, guards are already applied to the dictionary itself.
2. If dict_keys is used independently, its behavior is more like a set, we just need set similar guards.

This is why I introduced a new Dynamo variable rather than reuse the original DictKeys variable.

@yanboliang
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 18, 2024
@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch rebase origin/main returned non-zero exit code 1

Rebasing (1/1)
Auto-merging test/dynamo/test_functions.py
CONFLICT (content): Merge conflict in test/dynamo/test_functions.py
error: could not apply 2ed14a20721... [Dynamo] Add DictKeySetVariable to capture dict_keys passed outside of compiled region (#143374)
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Could not apply 2ed14a20721... [Dynamo] Add DictKeySetVariable to capture dict_keys passed outside of compiled region (#143374)
Details for Dev Infra team Raised by workflow job

[ghstack-poisoned]
[ghstack-poisoned]
@yanboliang
Copy link
Contributor 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants