Skip to content

detect duplicates in threads#1342

Merged
penelopeysm merged 2 commits intoTuringLang:mainfrom
hardik-xi11:fix/thread-safety
Apr 1, 2026
Merged

detect duplicates in threads#1342
penelopeysm merged 2 commits intoTuringLang:mainfrom
hardik-xi11:fix/thread-safety

Conversation

@hardik-xi11
Copy link
Copy Markdown
Contributor

final changes to close #1157

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 31, 2026

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.49%. Comparing base (2748445) to head (8ad8c92).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/accumulators/raw_values.jl 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
+ Coverage   78.43%   78.49%   +0.06%     
==========================================
  Files          50       50              
  Lines        3593     3608      +15     
==========================================
+ Hits         2818     2832      +14     
- Misses        775      776       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @hardik-xi11! I like it, just one minor comment. As usual, can you bump the patch and add a changelog too please.

Comment thread src/accumulators/raw_values.jl Outdated
Comment on lines +119 to +125
skel = DynamicPPL.VarNamedTuples.skeleton(acc2.values)
new_values = acc1.values
for (vn, val) in pairs(acc2.values)
top_sym = DynamicPPL.AbstractPPL.getsym(vn)
template_from_acc2_values = get(
skel.data, top_sym, DynamicPPL.VarNamedTuples.NoTemplate()
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think you need to make the skeleton here; you can just write template_from_acc2_vales = get(acc2.values.data, top_sym, NoTemplate()) since the actual value in the VNT doesn't matter, only the structure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's an interesting edge case here which is what should the template be if acc2 and acc1 have different templates, in other words, what happens if acc1 and acc2 both already contain x[1]. I think currently DynamicPPL will ignore the template and it will just use the structure inside acc1. That's not a problem in and of itself, but it might be worth a comment somewhere.

@hardik-xi11
Copy link
Copy Markdown
Contributor Author

everything should be good now

Copy link
Copy Markdown
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

Wonderful, thanks!

@penelopeysm penelopeysm merged commit 1c369aa into TuringLang:main Apr 1, 2026
21 of 22 checks passed
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.

Thread safety in check_model is handled poorly

2 participants