Skip to content

wconstab/38034-sliced-sequential #40445

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

wconstab/38034-sliced-sequential #40445

wants to merge 1 commit into from

Conversation

wconstab
Copy link
Contributor

@wconstab wconstab commented Jun 23, 2020 •
edited
Loading

Partial support for slicing of Sequential containers.

  • works around missing Sequential slice functionality
    by converting to tuple
  • only supports iteration of resulting tuple values,
    not direct call() on the sliced sequential

@wconstab wconstab requested a review from apaszke as a code owner June 23, 2020 17:49
@wconstab wconstab self-assigned this Jun 23, 2020
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 23, 2020
@dr-ci
Copy link

dr-ci bot commented Jun 23, 2020 •
edited
Loading

💊 CI failures summary and remediations

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

  • 1/1 failures introduced in this PR

XLA failure

Job pytorch_xla_linux_bionic_py3_6_clang9_build is failing. Please create an issue with title prefixed by [PT_BREAK] in pytorch/xla and link to to this PR. If you have questions, please reach out to @ailzhang / @dlibenzi / @JackCaoG.

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 19 times.

eellison
eellison approved these changes Jun 23, 2020
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

There are a couple clean ups that we should do, but I think this is worth merging for the release because there is no fix here short of making an O(n) iteration into O(n^2).

torch/csrc/jit/python/python_sugared_value.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you error if this isn't a ModuleList here ? nn.ModuleDict doesn't work either (no handling of slices) https://github.com/pytorch/pytorch/blob/master/torch/nn/modules/container.py#L284

torch/csrc/jit/frontend/ir_emitter.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add: TODO @wconstab refactor using Symbol instead of string for comparison here

and maybe some context on why we are intercepting the slicing for modulelists/sequentials here / normal code emission won't work. (doesn't have to be too long).

torch/csrc/jit/frontend/sugared_value.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

another TODO: @wconstab refactor using asTuple

@wconstab wconstab changed the title [WIP] Wconstab/38034 repro wconstab/38034 Jun 23, 2020
facebook-github-bot
facebook-github-bot reviewed Jun 23, 2020
View reviewed changes
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.

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

@ezyang
Copy link
Contributor

ezyang commented Jun 23, 2020

You should probably rebase this to a green master. Also, try to have a more descriptive PR title in the future

@wconstab wconstab changed the title wconstab/38034 wconstab/38034-sliced-sequential Jun 23, 2020
@wconstab wconstab force-pushed the wconstab/38034-repro branch from 4638860 to e9ceb44 Compare June 23, 2020 21:24
facebook-github-bot
facebook-github-bot reviewed Jun 23, 2020
View reviewed changes
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.

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

@wconstab
Allow slicing sequential container
d52062e 
- fixes pytorch#38034
- works around missing slice functionality in Sequential
  by casting to tuple and slicing that instead
- supports iterating on the resulting slice but not call()
@wconstab wconstab force-pushed the wconstab/38034-repro branch from e9ceb44 to d52062e Compare June 23, 2020 21:27
facebook-github-bot
facebook-github-bot reviewed Jun 23, 2020
View reviewed changes
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.

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

@wconstab
Copy link
Contributor Author

These failures look OK to me.
Reran failing buck tests and they passed, seemed unrelated as well.
Facebook Internal - Builds & Tests Action required
And not sure if necessary to rerun xla build, but the failure looks totally unrelated.
ci/circleci: pytorch_xla_linux_bionic_py3_6_clang9_build — Your tests failed on CircleCI

@facebook-github-bot
Copy link
Contributor

@wconstab merged this pull request in d855528.

@wconstab wconstab mentioned this pull request Jun 30, 2020
Merged
@ppwwyyxx ppwwyyxx mentioned this pull request Mar 5, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@eellison eellison eellison approved these changes

@apaszke apaszke Awaiting requested review from apaszke

Assignees

@wconstab wconstab

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wconstab @ezyang @facebook-github-bot @eellison @mruberry