(fix): USE_POOLING=false path fixes and 2-tier toggle rename#24
Merged
(fix): USE_POOLING=false path fixes and 2-tier toggle rename#24
USE_POOLING=false path fixes and 2-tier toggle rename#24Conversation
…alse tests
- DisabledPool{Backend} now subtypes AbstractArrayPool, so functions
typed as `pool::AbstractArrayPool` accept DisabledPool when pooling
is disabled. Previously, passing a DisabledPool to typed sub-functions
would error.
- Remove generic DisabledPool{B} fallbacks with `args...` that caused
method ambiguity with AbstractArrayPool methods (14 Aqua violations).
- Remove dead _impl! delegators for DisabledPool — macros always use
the original expr (with public acquire!) in the disabled branch,
never the transformed _acquire_impl! path.
- Rewrite test_disabled_pooling.jl with 9 comprehensive test scenarios:
type hierarchy, sub-function passing with ::AbstractArrayPool type
annotations, all convenience functions, Bit type, state no-ops,
function-def mode, and multi-dimensional arrays.
…ests
- Add reshape! methods for DisabledPool{:cuda} (parity with CPU)
- Add sub-function passing tests (untyped, ::AbstractArrayPool, nested)
- Add state management no-ops test (checkpoint!, rewind!, reset!, empty!)
- Add reshape! dispatch test for disabled CUDA pool
…NG_ENABLED[] _generate_function_pool_code_with_backend was missing force_enable parameter, causing @maybe_with_pool :backend function definitions to always use the real pool — ignoring the runtime toggle entirely. - Add force_enable parameter to _generate_function_pool_code_with_backend - Generate MAYBE_POOLING_ENABLED[] branch when force_enable=false - Add @maybe_with_pool :backend macro expansion tests (block/function/short-form) - Add USE_POOLING=false subprocess tests: function+helper passing, @maybe_with_pool function - Update test_coverage.jl for new function signature
…→ MAYBE_POOLING
Clarify the 2-tier pooling toggle hierarchy:
- Tier 1: STATIC_POOLING (compile-time const, master switch)
- Tier 2: MAYBE_POOLING (runtime Ref{Bool}, @maybe_with_pool only)
Old names kept as simple const aliases for backward compatibility.
Preference key "use_pooling" in LocalPreferences.toml unchanged.
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness gaps in the STATIC_POOLING=false / pooling-disabled paths and clarifies the two-tier pooling toggles by renaming the compile-time and runtime switches (while keeping backward-compatible aliases).
Changes:
- Rename toggles to
STATIC_POOLING(compile-time master switch) andMAYBE_POOLING(runtime toggle for@maybe_with_pool), keepingUSE_POOLING/MAYBE_POOLING_ENABLEDas aliases. - Make
DisabledPoolsubtypeAbstractArrayPool, add missing CUDAreshape!fallback, and fix@maybe_with_pool :backendfunction-form runtime toggle behavior. - Expand and adjust tests to cover the pooling-disabled branches (including separate-process preference tests and macro-expansion checks).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_task_local_pool.jl | Update runtime toggle tests to use MAYBE_POOLING[]. |
| test/test_reshape.jl | Update reshape allocation tests to use MAYBE_POOLING[]. |
| test/test_macros.jl | Update macro behavior tests to use MAYBE_POOLING[]. |
| test/test_macro_expansion.jl | Update macro expansion assertions/comments for renamed runtime toggle. |
| test/test_disabled_pooling.jl | Major expansion of separate-process STATIC_POOLING=false coverage; improved failure diagnostics. |
| test/test_coverage.jl | Adjust coverage expectations for disabled/unknown backend behavior and updated generator signature. |
| test/test_bitarray.jl | Update runtime toggle references and remove tests tied to removed generic DisabledPool error paths. |
| test/test_backend_macro_expansion.jl | Add macroexpansion tests for @maybe_with_pool :backend runtime toggle checks. |
| test/cuda/test_disabled_pool.jl | Add CUDA disabled-pool tests for reshape!, sub-function passing, and no-op state ops. |
| src/types.jl | DisabledPool{Backend} <: AbstractArrayPool and updated docs to new toggle names. |
| src/task_local_pool.jl | Introduce STATIC_POOLING/MAYBE_POOLING and keep deprecated aliases. |
| src/macros.jl | Fix USE_POOLING=false → STATIC_POOLING=false gating and ensure backend function-form honors runtime toggle. |
| src/legacy/types.jl | Make legacy DisabledPool subtype AbstractArrayPool. |
| src/legacy/acquire.jl | Remove generic DisabledPool fallbacks/delegators that no longer apply with new macro codegen. |
| src/convenience.jl | Remove generic DisabledPool fallbacks/delegators (consistent with new codegen paths). |
| src/acquire.jl | Remove generic DisabledPool fallbacks/delegators (consistent with new codegen paths). |
| src/AdaptiveArrayPools.jl | Export new toggle names and keep deprecated exports for compatibility. |
| ext/AdaptiveArrayPoolsCUDAExt/convenience.jl | Add reshape! overloads for DisabledPool{:cuda} and update docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ompat - Add _record_type_touch!(::DisabledPool, ::Type) = nothing to both acquire.jl and legacy/acquire.jl (DisabledPool has no state to track) - Replace FieldError with Exception in test_coverage.jl (FieldError is Julia 1.11+, Julia 1.10 throws ErrorException for missing fields) - Fix @maybe_with_pool docstring: disabled pool is DisabledPool, not nothing - Fix subprocess stderr capture in test_disabled_pooling.jl: capture stdout/stderr via IOBuffer in single run instead of re-executing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #24 +/- ##
==========================================
+ Coverage 95.71% 95.92% +0.21%
==========================================
Files 13 13
Lines 1820 1768 -52
==========================================
- Hits 1742 1696 -46
+ Misses 78 72 -6
🚀 New features to boost your workflow:
|
mgyoo86
referenced
this pull request
Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix bugs in the
USE_POOLING=falsecode path and clarify the 2-tier pooling toggle naming.Changes
Bug fixes
DisabledPool <: AbstractArrayPool— fixes type checks failing underUSE_POOLING=falsereshape!fallback forDisabledPool{:cuda}@maybe_with_pool :backendfunction form ignoringMAYBE_POOLING_ENABLED[]Refactor: 2-tier pooling toggle rename
USE_POOLING→STATIC_POOLING(compile-time const, master switch)MAYBE_POOLING_ENABLED→MAYBE_POOLING(runtimeRef{Bool},@maybe_with_poolonly)constaliases for backward compatibilityLocalPreferences.tomlkeyuse_poolingunchangedTests
USE_POOLING=falseseparate-process tests (290+ lines)@maybe_with_pool :backendmacro expansion testsDisabledPooltestsBreaking changes
None — existing code using
USE_POOLINGandMAYBE_POOLING_ENABLEDcontinues to work.