Skip to content
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

Fix out of memory bug #1270 #1286

Merged
merged 2 commits into from Feb 12, 2024
Merged

Conversation

antimora
Copy link
Collaborator

@antimora antimora commented Feb 9, 2024

Pull Request Template

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

#1270

Changes

  1. Default value sequence generation uses a bound. This caused OOM in cases when a field was not present in .pt file.
  2. Panic if a struct field is missing from pt file.
  3. Add a new regression test.

Testing

Existing unit tests are passing and new test was added.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 214 lines in your changes are missing coverage. Please review.

Comparison is base (9df2071) 84.41% compared to head (3e9c6cf) 84.48%.
Report is 25 commits behind head on main.

Files Patch % Lines
backend-comparison/src/burnbenchapp/base.rs 0.00% 58 Missing ⚠️
burn-dataset/src/vision/image_folder.rs 79.91% 49 Missing ⚠️
burn-wgpu/src/codegen/dialect/gpu/vectorization.rs 77.66% 23 Missing ⚠️
backend-comparison/src/burnbenchapp/term/base.rs 0.00% 17 Missing ⚠️
burn-wgpu/src/codegen/dialect/wgsl/base.rs 83.50% 16 Missing ⚠️
burn-wgpu/src/fusion/elemwise/builder.rs 93.83% 14 Missing ⚠️
burn-tch/src/ops/int_tensor.rs 0.00% 9 Missing ⚠️
burn-wgpu/src/codegen/dialect/wgsl/operations.rs 86.66% 4 Missing ⚠️
burn-wgpu/src/codegen/dialect/wgsl/shader.rs 33.33% 4 Missing ⚠️
backend-comparison/src/bin/burnbench.rs 0.00% 3 Missing ⚠️
... and 9 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1286      +/-   ##
==========================================
+ Coverage   84.41%   84.48%   +0.06%     
==========================================
  Files         549      561      +12     
  Lines       61952    62774     +822     
==========================================
+ Hits        52295    53032     +737     
- Misses       9657     9742      +85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@laggui laggui left a comment

Choose a reason for hiding this comment

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

LGTM! Just some minor comments related to typos 👍

burn-core/src/record/serde/data.rs Outdated Show resolved Hide resolved
burn-core/src/record/serde/de.rs Outdated Show resolved Hide resolved
@antimora antimora requested a review from laggui February 9, 2024 21:29
Comment on lines +460 to +463
struct DefaultDeserializer {
/// The originator field name (the top level missing field name)
originator_field_name: Option<String>,
}
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we consider renaming that struct at some point. Returning the default value isn't accurate for all deserialization types. The current name primarily defines 'how' it works, but it doesn't precisely convey 'what' task is performed by deserializing default values. My understanding is that the objective is to extract an empty data structure tree, creating any 'container' node with default values, while the leaf node should not undergo deserialization (deserialize_struct). Am I correct?

Copy link
Collaborator Author

@antimora antimora Feb 10, 2024

Choose a reason for hiding this comment

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

Yes, it has become hybrid now. Initially, I was thinking it would be ok to fill-in with default values for all unmatched "tree" branches. Although it works, it leads to odd outcomes (module with default values) if the field name mismatched. So here instead I opted to throw an error (field name + type).

I am open for a better name. We have some time to think since this is a private deserializer struct.

@nathanielsimard nathanielsimard merged commit 16541ea into tracel-ai:main Feb 12, 2024
14 checks passed
@nathanielsimard nathanielsimard deleted the bug-1270 branch February 12, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants