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

[JIT] fix specialized list from dict keys #23267

Closed
wants to merge 2 commits into from

Conversation

eellison
Copy link
Contributor

Previously we weren't specializing the list returned from dict.keys()

@eellison eellison requested a review from driazati July 23, 2019 21:18
@pytorchbot pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jul 23, 2019
template <>
c10::impl::GenericList makeListForDictValues<IValue>(
template <unsigned int Index>
c10::impl::GenericList makeGenericListForDictKeysOrValues(
const std::vector<std::pair<IValue, IValue>>& order) {
auto values = c10::impl::GenericList(c10::impl::deprecatedUntypedList());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to do c10::List<IValue> so we don't have to have a separate copy of this function for GenericLists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that doesn't compile. And you can't do partial template specialization so that you are only passing in the Index in

test/test_jit.py Outdated
def specialized_list():
return {1: 1, 2: 2}.keys()

self.assertTrue(set(specialized_list()) == set([1, 2]))
Copy link
Contributor

Choose a reason for hiding this comment

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

This would have passed with deprecatedUntypedList before, how does it test the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now it is testing an op with a specialized list. i don't know what deprecatedUntypedList is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay added op that uses specialized list

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@eellison merged this pull request in 1de44a6.

facebook-github-bot pushed a commit that referenced this pull request Jul 27, 2019
Summary:
Add `sorted` keyword to JIT for lists and dicts. This desugars to a list copy and a call to `list.sort()`. Since we don't have interfaces yet I implement it in terms of `list.sort()`. When we do we can re-visit implementing this op in a different manner.

The test fails bc of a fix to specialized lists which is landing here: #23267

Ignore the first commit because it is formatting, plz use clang_format ppl :'(
Pull Request resolved: #23274

Differential Revision: D16527323

Pulled By: eellison

fbshipit-source-id: aed8faef23cb790b9af036cd6c1b9b1d7066345d
zdevito pushed a commit to zdevito/ATen that referenced this pull request Jul 27, 2019
Summary:
Add `sorted` keyword to JIT for lists and dicts. This desugars to a list copy and a call to `list.sort()`. Since we don't have interfaces yet I implement it in terms of `list.sort()`. When we do we can re-visit implementing this op in a different manner.

The test fails bc of a fix to specialized lists which is landing here: pytorch/pytorch#23267

Ignore the first commit because it is formatting, plz use clang_format ppl :'(
Pull Request resolved: pytorch/pytorch#23274

Differential Revision: D16527323

Pulled By: eellison

fbshipit-source-id: aed8faef23cb790b9af036cd6c1b9b1d7066345d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants