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

Remove transformPreLowering() #2365

Merged
merged 1 commit into from Feb 9, 2019

Conversation

Projects
None yet
3 participants
@jfix71
Copy link
Contributor

jfix71 commented Feb 7, 2019

Description: As mentioned in #2339, I don't think we need transformPreLowering() anymore.

  1. I don't think any of our backends are using this functionality given the ability to prevent lowering via shouldLower(), and then later transform the graph during transformPostLowering().
  2. Simplify the compilation pipeline since it doesn't seem useful/used.
  3. Now that #2311 has landed, when quantizing we lower prior to transformPreLowering() anyway, so it's a misleading function and could cause bugs and/or confusion.

Testing: All unit tests still pass.

Documentation: Updated.

Closes #2339

@rdzhabarov
Copy link
Contributor

rdzhabarov left a comment

oh, there were no tests exercising transformPreLowering functionality ...
LGTM.

@jfix71

This comment has been minimized.

Copy link
Contributor Author

jfix71 commented Feb 8, 2019

Yeah we aren't explicitly testing this functionality anywhere. Perhaps we could add some tests with MockBackends for shouldLower(), transformPostLowering(), etc.

@jfix71 jfix71 merged commit 8929a8c into pytorch:master Feb 9, 2019

6 checks passed

ci/circleci: ASAN Your tests passed on CircleCI!
Details
ci/circleci: DEBUG Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_WITH_LIT Your tests passed on CircleCI!
Details
ci/circleci: SHARED Your tests passed on CircleCI!
Details
ci/circleci: TSAN Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jfix71 jfix71 deleted the jfix71:remove_transform_pre_lowering branch Feb 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment