[ONNX] Track and list model params for scripting #47348
Conversation
facebook-github-bot
added
cla signed
oncall: jit
💊 CI failures summary and remediationsAs of commit 90309a6 (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. This comment has been revised 162 times. |
ejguan
added
module: onnx
| for (Block* sub_block : n->blocks()) { | ||
| blocks.emplace_back(sub_block); | ||
| } | ||
| if (n->kind() == prim::SetAttr) { |
There was a problem hiding this comment.
What is difference between prim::SetAttr and prim::GetAttr types?
There was a problem hiding this comment.
SetAttr writes to an attribute.
Codecov Report
@@ Coverage Diff @@
## master #47348 +/- ##
==========================================
+ Coverage 74.28% 80.73% +6.45%
==========================================
Files 1858 1863 +5
Lines 200305 200529 +224
==========================================
+ Hits 148787 161889 +13102
+ Misses 51518 38640 -12878 |
Co-authored-by: Bowen Bao <imbowenbao@outlook.com>
…oof/scripting_params
…oof/scripting_params
…rch into neraoof/scripting_params
…oof/scripting_params # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
|
|
||
| std::string fullName("self_"); | ||
| for (auto& name : moduleNames) { | ||
| fullName += name + '_'; |
There was a problem hiding this comment.
please ensure parameter names are the same as named_parameters(). Can we add tests in test_utility?
There was a problem hiding this comment.
So named_parameters() and named_buffers() do not have the "self." prefix as part of the name.
So param name can be: %self.conv.weight for a named_parameter conv.weight.
Do you think to remove "self." from param names?
There was a problem hiding this comment.
Yes, as long as they match with named_parameters() and named_buffers().
…oof/scripting_params
…oof/scripting_params # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
neginraoof
force-pushed
the
neraoof/scripting_params
branch
2 times, most recently
from
8fb4ae8
to
65ecd22
Compare
…oof/scripting_params
…rch into neraoof/scripting_params
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@neginraoof This PR could be causing failures Is it possible that you forgot to update torch/_C/init.pyi.in to include that param in _freeze_module interface? |
|
Apologies, this is not fault of this PR, but the issue is with #48782 which added outdated mypy annotations. |
List model parameters as inputs following freezing script module.