-
Notifications
You must be signed in to change notification settings - Fork 440
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
Feat/lazy init #1539
Feat/lazy init #1539
Conversation
895f3e0
to
e9f32da
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1539 +/- ##
==========================================
+ Coverage 86.30% 86.48% +0.17%
==========================================
Files 684 684
Lines 78216 78093 -123
==========================================
+ Hits 67502 67535 +33
+ Misses 10714 10558 -156 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just once sentence to fix
/// To avoid creating locks on already initialized parameter, we wrap the lock inside an | ||
/// Option, the inner option is required for resetting the state onced initialized. | ||
/// TLDR: RwLock(None) only happens on the param reference that is lazy, but was initialized, | ||
/// all other parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't parse this sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the last sentence is incomplete 😅
all other parameters [blank].. what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation LGTM!
The examples have been updated but I think we're missing changes to the book, no? We should reflect that.
/// To avoid creating locks on already initialized parameter, we wrap the lock inside an | ||
/// Option, the inner option is required for resetting the state onced initialized. | ||
/// TLDR: RwLock(None) only happens on the param reference that is lazy, but was initialized, | ||
/// all other parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the last sentence is incomplete 😅
all other parameters [blank].. what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I had one question you should see.
pub fn init_with<B: Backend>(&self, record: BenchmarkModuleRecord<B>) -> BenchmarkModule<B> { | ||
BenchmarkModule { | ||
linears: record | ||
.linears | ||
.into_iter() | ||
.map(|record| nn::Linear { | ||
weight: record.weight, | ||
bias: record.bias, | ||
}) | ||
.collect(), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting rid off init_with
and adding in other places? ;-)
let running_mean = Tensor::zeros([self.num_features], device); | ||
let running_var = Tensor::ones([self.num_features], device); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why it's different for running*?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update the book?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Minor phrasing comments but approving in advance because it's nothing critical.
You need two things in order to load weights for a model: the model's weights and the model's | ||
config. Since parameters in Burn are lazy initialized, no allocation and GPU/CPU kernels are executed by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rephrase to avoid using "weights" too many times?
Two things are required to load a model's weights: the model's record and the model's config.
@@ -40,29 +40,18 @@ fn main() { | |||
|
|||
## Good practices | |||
|
|||
By using the Config pattern it is easy to create instances from this | |||
config. Therefore, initialization methods should be implemented on the config struct. | |||
By using the config type it is easy to create new module instances from it. The initialization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create new module instances from "it" sounds a bit weird. I think we can simply leave this out.
By using the config type it is easy to create new module instances.
Fix #1329