-
Notifications
You must be signed in to change notification settings - Fork 580
Document defaults to with xxx methods #3151
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
base: main
Are you sure you want to change the base?
Document defaults to with xxx methods #3151
Conversation
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.
Looks like there are some issues with the modified config derive (see CI).
@@ -149,104 +149,179 @@ impl ConfigStructAnalyzer { | |||
|
|||
impl ConfigAnalyzer for ConfigStructAnalyzer { | |||
fn gen_new_fn(&self) -> TokenStream { |
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.
The generated new
method seems to be invalid.
crates/burn-core/src/nn/conv/conv2d.rs:17:10:
called `Result::unwrap()` on an `Err` value: Error("expected value", line: 1, column: 1)
Seems to originate from the serde_json::from_str(#value_str).unwrap()
. The values are not captured properly it seems.
This also seems to be reflected in the docstrings, e.g.
Create a new instance of the config.
Fields:
#_name: #_doc
#_name: #_doc
#_name: #_doc Default: #_value_str
#_name: #_doc Default: #_value_str
#_name: #_doc Default: #_value_str
#_name: #_doc Default: #_value_str
#_name: #_doc Default: #value
#_name: #_doc Default: #_value_str
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.
While it now seems to at least compile, the docstring still remains incorrect it.
Have you tested it on your end to see if it correctly parses and displays the fields?
I think we can keep this simple without the need for serde_json
. Perhaps just check if field.doc()
contains a docstring and work from there.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3151 +/- ##
==========================================
- Coverage 82.14% 82.14% -0.01%
==========================================
Files 959 959
Lines 121990 122167 +177
==========================================
+ Hits 100213 100356 +143
- Misses 21777 21811 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -15,3 +15,4 @@ burn = {path = "../../crates/burn", features = ["webgpu", "train", "vision"]} | |||
# Serialization | |||
log = {workspace = true} | |||
serde = {workspace = true, features = ["std", "derive"]} | |||
serde_json = {workspace = true, features = ["std"]} |
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.
This additional dependency is not desired for the config derive usage. It also breaks no-std usage.
@@ -149,104 +149,179 @@ impl ConfigStructAnalyzer { | |||
|
|||
impl ConfigAnalyzer for ConfigStructAnalyzer { | |||
fn gen_new_fn(&self) -> TokenStream { |
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.
While it now seems to at least compile, the docstring still remains incorrect it.
Have you tested it on your end to see if it correctly parses and displays the fields?
I think we can keep this simple without the need for serde_json
. Perhaps just check if field.doc()
contains a docstring and work from there.
Pull Request Template
Checklist
cargo run-checks
command has been executed.Tip
Want more detailed macro error diagnostics? This is especially useful for debugging tensor-related tests:
RUSTC_BOOTSTRAP=1 RUSTFLAGS="-Zmacro-backtrace" cargo run-checks
Related Issues/PRs
#2982
Changes
The Config derive macro was missing automatic documentation generation for config structs. This meant that:
new
functionI added automatic documentation generation to the Config derive macro that:
new
function, creating a comprehensive list of all fields with their descriptionsTesting
Unit tests in
burn-core/tests/test_derive_config.rs
:test_config_documentation()
verifies that: