fix: preserve symbolic external units across precompile#215
fix: preserve symbolic external units across precompile#215MilesCranmerBot wants to merge 14 commits into
Conversation
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
Benchmark Results (Julia v1.10)Time benchmarks
Memory benchmarks
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #215 +/- ##
==========================================
+ Coverage 99.15% 99.18% +0.02%
==========================================
Files 23 23
Lines 1305 1347 +42
==========================================
+ Hits 1294 1336 +42
Misses 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@MilesCranmerBot we need to get to 100% coverage of the diff please |
|
@MilesCranmerBot code coverage is not yet 100% for the diff. And also see comment about making breaking changes please. |
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
@MilesCranmerBot needs to add a test where we have a fake package that registers units, and that has a function which uses those units. See the recent issue for the workflow I'd like to enable. |
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
Done. I pushed this to the PR branch in commit 7645686. What changed:
Validation:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7645686b00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@MilesCranmerBot Please address the above feedback |
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
Addressed the review feedback in 524cb15. What changed:
Validation:
|
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Hm, I still think this feels too hacky... @MilesCranmerBot could you please change the PR in the following way? Here's a detailed description:
|
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
@MilesCranmerBot CI is failing, please fix. |
|
@MilesCranmerBot code coverage of the diff is under 100%. Are there methods which aren't possible to hit in a realistic scenario? If so then please remove them. If it is possible to hit them, then ensure 100% diff coverage with testing. |
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
Addressed this in a8912ae. The uncovered diff lines were fallback branches in I reran |
MilesCranmer
left a comment
There was a problem hiding this comment.
@MilesCranmerBot see new comments
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
Addressed in 6efcab1. What changed:
Validation:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6efcab19a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
MilesCranmer
left a comment
There was a problem hiding this comment.
@MilesCranmerBot Please address these comments + those of codex bot + ensure that code coverage for the diff is 100% (failing CI at the moment)
Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
|
Addressed the open review comments in 9f886d9. What changed:
Local verification:
I also resolved the review threads that this commit addresses. CI is running on the updated branch now, including the codecov path. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f886d9c55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elseif has_external_binding | ||
| return Expr(:call, GlobalRef(@__MODULE__, :lookup_external_unit), QuoteNode(mod), QuoteNode(sym)) |
There was a problem hiding this comment.
Avoid external fallback for registered unit symbols
When a caller module does using DynamicQuantities, built-in units like m satisfy both has_registered_binding and has_external_binding, so this branch routes u"..." through lookup_external_unit instead of lookup_unit. That forces ensure_registered_external_unit (and its global UNIT_UPDATE_LOCK) on every unit-literal evaluation, turning previously constant-like macro expansions into synchronized runtime lookups and degrading threaded hot paths. The external fallback should only run when the registered lookup is actually missing.
Useful? React with 👍 / 👎.
Summary
Preserve external symbolic unit registrations across precompile and import boundaries.
Changes
us"..."symbolic macro expansionTesting
/usr/local/bin/julia --project -e 'using Pkg; Pkg.test()'Fixes #212