Skip to content

Conversation

KsenijaS
Copy link
Contributor

@KsenijaS KsenijaS commented May 20, 2020

This PR:

  • Adds eliminate_unused_items pass that removes unused inputs and initializers.
  • Fixes run_embed_params function so it doesn't export unnecessary parameters.
  • Removes test_modifying_params in test_verify since it's no longer needed.

@KsenijaS KsenijaS requested a review from apaszke as a code owner May 20, 2020 20:50
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label May 20, 2020
@KsenijaS
Copy link
Contributor Author

@BowenBao @neginraoof please review

@dr-ci
Copy link

dr-ci bot commented May 20, 2020

💊 CI failures summary and remediations

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


💚 💚 Looks good so far! There are no failures yet. 💚 💚


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 76 times.

@ailzhang ailzhang requested review from BowenBao and neginraoof May 21, 2020 16:37
@ailzhang ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 21, 2020
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

A high level question, won't general lint pass and dce pass work here?

@KsenijaS
Copy link
Contributor Author

KsenijaS commented Jun 2, 2020

dce pass doesn't remove unused initializers and the only unused inputs that it removes are from prim:fork node: https://github.com/pytorch/pytorch/blob/master/torch/csrc/jit/passes/dead_code_elimination.cpp#L51

Copy link
Collaborator

@BowenBao BowenBao left a comment

Choose a reason for hiding this comment

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

minor comment inline, LGTM

@KsenijaS
Copy link
Contributor Author

@houseroad can you please review this.

@KsenijaS
Copy link
Contributor Author

@houseroad flake8 and ci.pytorch.org test failure are not caused by this PR

@KsenijaS
Copy link
Contributor Author

@houseroad clang-tidy failure is because of onnxTypeToScalarTypeMap in constant_fold pass:
https://github.com/pytorch/pytorch/blob/master/torch/csrc/jit/passes/onnx/constant_fold.cpp#L17

Do you have any other comment or can we merge this PR?

@houseroad
Copy link
Member

Do you want to create some constant to address the issue?

@KsenijaS
Copy link
Contributor Author

KsenijaS commented Jun 17, 2020

@houseroad I added constants ::ONNX_NAMESPACE::TensorProto_DataType_* to replace numbers 1 - 12 in onnxTypeToScalarTypeMap inside constant folding pass that were causing clang-tidy failure on the CI. However clang-tidy does not have the generated onnx header so it will still fail on the CI.
Do you want me to revert to previous state or keep the current code?

@KsenijaS
Copy link
Contributor Author

@houseroad can we merge this PR?

Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Can we let clang-tidy pass?

@BowenBao
Copy link
Collaborator

@houseroad please have a look, the clang-tidy issue is resolved. :)

@KsenijaS
Copy link
Contributor Author

@houseroad CI is green, can we merge this PR?

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.

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

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.

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

@facebook-github-bot
Copy link
Contributor

@houseroad merged this pull request in 547ea78.

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 open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants