Implement C++ ModuleDict #47707
Implement C++ ModuleDict #47707
Conversation
[ghstack-poisoned]
ejguan
requested review from
ebetica,
glaringlee,
goldsborough and
yf225
as code owners
💊 CI failures summary and remediationsAs of commit 8716117 (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 16 times. |
Fixes #45896 - [ ] Add tests Differential Revision: [D24872641](https://our.internmc.facebook.com/intern/diff/D24872641) [ghstack-poisoned]
Fixes #45896 - [ ] Add tests Differential Revision: [D24872641](https://our.internmc.facebook.com/intern/diff/D24872641) [ghstack-poisoned]
Codecov Report
@@ Coverage Diff @@
## gh/ejguan/8/base #47707 +/- ##
====================================================
+ Coverage 81.26% 81.27% +0.01%
====================================================
Files 1839 1841 +2
Lines 198808 199016 +208
====================================================
+ Hits 161554 161746 +192
- Misses 37254 37270 +16 |
zou3519
left a comment
zou3519
left a comment
There was a problem hiding this comment.
Not many comments from me but I don't know much about the C++ API
| /// `reset()` is empty for `ModuleDict`, since it does not have parameters of | ||
| /// its own. | ||
| void reset() override {} |
There was a problem hiding this comment.
@glaringlee how is reset() used in the C++ API? If it doesn't make sense to call reset() for ModuleDict, would it be better to raise an error?
There was a problem hiding this comment.
@zou3519
Can not raise an error, since this is called in cloneable.h when cloning, even it is empty, it will be called. If reset() is not used here, leaving it empty is fine. I somehow recalled that reset() in this moduledict case should do something special, will update here.
There was a problem hiding this comment.
@zou3519 I reviewed a bit, clone() is also overridden in this impl, so reset() is actually useless. I think it is fine to just leave it as blank, this is kinda convention in c++ API impls.
|
And, I am wondering if we need another API of |
I did not see any usage for ptr at the moment, but you can add it if it is not heavy work. |
Fixes #45896 - [x] API * constructor(OrderedDict/vector of pair) * clear() * items() * keys() * pop(string key) * update(OrderedDict/vector of pair/ModuleDict) * values() * Iterator: begin/end * size() * empty() * contains(string key) * clone() * pretty_print(stream) * operator [string key] * at(string key) - [x] Add tests Differential Revision: [D24872641](https://our.internmc.facebook.com/intern/diff/D24872641) [ghstack-poisoned]
glaringlee
left a comment
glaringlee
left a comment
There was a problem hiding this comment.
Just one comment, I should mention earlier, sry.
Otherwise, all looks good to me.
Fixes #45896 - [x] API * constructor(OrderedDict/vector of pair) * clear() * items() * keys() * pop(string key) * update(OrderedDict/vector of pair/ModuleDict) * values() * Iterator: begin/end * size() * empty() * contains(string key) * clone() * pretty_print(stream) * operator [string key] * at(string key) - [x] Add tests Differential Revision: [D24872641](https://our.internmc.facebook.com/intern/diff/D24872641) [ghstack-poisoned]
Stack from ghstack:
Fixes #45896
Differential Revision: D24872641