Skip to content

Scope binning memory resource fixture to class in Python tests#2284

Merged
rapids-bot[bot] merged 2 commits intorapidsai:mainfrom
bdice:scope-binning-mr-fixture
Mar 11, 2026
Merged

Scope binning memory resource fixture to class in Python tests#2284
rapids-bot[bot] merged 2 commits intorapidsai:mainfrom
bdice:scope-binning-mr-fixture

Conversation

@bdice
Copy link
Copy Markdown
Collaborator

@bdice bdice commented Mar 10, 2026

Description

Follow-up to #2273. The binning memory resource test recreates a BinningMemoryResource (with FixedSizeMemoryResource bins backed by ManagedMemoryResource) for every (dtype, nelem, alloc, upstream_mr) combination. The managed memory slab allocations make each creation take ~0.5–0.9s, totaling ~40s for 147 tests.

Restructure the test into a class with a class-scoped fixture so that each BinningMemoryResource is built once per upstream_mr rather than once per test. A function-scoped autouse fixture sets the current device resource before each test.

Times measured on an RTX 2070 Super (8 GiB) under WSL2, where managed memory operations can be particularly expensive. Binning tests drop from ~39s to ~3s; full suite drops from ~49s to ~11s.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

The binning memory resource test recreates a BinningMemoryResource
(with FixedSizeMemoryResource bins backed by ManagedMemoryResource)
for every (dtype, nelem, alloc, upstream_mr) combination. The managed
memory slab allocations make each creation take ~0.5-0.9s, totaling
~40s for 147 tests.

Restructure the test into a class with a class-scoped fixture so that
each BinningMemoryResource is built once per upstream_mr rather than
once per test. A function-scoped autouse fixture re-sets the current
device resource before each test.

Times measured on an RTX 2070 Super (8 GiB) under WSL2, where managed
memory operations can be particularly expensive. Binning tests drop
from ~39s to ~3s; full suite drops from ~49s to ~11s.
@bdice bdice requested a review from a team as a code owner March 10, 2026 23:25
@bdice bdice added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Mar 10, 2026
@bdice bdice self-assigned this Mar 10, 2026
@bdice bdice moved this to In Progress in RMM Project Board Mar 10, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d2bda107-a6b6-4bf7-83cc-41f67b288635

📥 Commits

Reviewing files that changed from the base of the PR and between db4f3f6 and 475a745.

📒 Files selected for processing (1)
  • python/rmm/rmm/tests/test_binning_memory_resource.py

📝 Walkthrough

Summary by CodeRabbit

  • Tests
    • Refactored binning memory resource tests to a class-based structure using parameterized fixtures so each upstream memory resource is tested separately.
    • Added helper setup to construct per-upstream test resources and an automatic per-test setup to ensure the correct device resource is active.
    • Consolidated assertions into a class method that consumes the new fixtures.

Walkthrough

Refactors tests for BinningMemoryResource: adds helper _make_binning_mr(upstream_mr), introduces _UPSTREAM_MRS list, replaces top-level parametrized test with TestBinningMemoryResource class, and adds a class-scoped binning_mr fixture plus an autouse device-setup fixture.

Changes

Cohort / File(s) Summary
Test Refactoring
python/rmm/rmm/tests/test_binning_memory_resource.py
Added _make_binning_mr() helper, _UPSTREAM_MRS list, class-scoped @pytest.fixture binning_mr (parameterized over upstream factories), converted top-level parametrized test into class TestBinningMemoryResource with autouse device setup and test_binning_memory_resource method.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • TomAugspurger
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: restructuring a test to use a class-scoped fixture for the binning memory resource, which is the primary focus of the refactoring.
Description check ✅ Passed The description is directly related to the changeset, explaining the motivation (test performance improvement), the approach (class-scoped fixture), and providing measurable impact data.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Co-authored-by: Matthew Murray <41342305+Matt711@users.noreply.github.com>
@bdice
Copy link
Copy Markdown
Collaborator Author

bdice commented Mar 11, 2026

/merge

@rapids-bot rapids-bot bot merged commit b43680c into rapidsai:main Mar 11, 2026
77 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in RMM Project Board Mar 11, 2026
@bdice bdice mentioned this pull request Mar 11, 2026
3 tasks
bdice added a commit to bdice/rmm that referenced this pull request Mar 12, 2026
…tests

Scope the FixedSizeMemoryResource fixture to the test class so
the resource is created once per upstream rather than once per
(dtype, nelem, alloc) combination, matching the pattern established
for BinningMemoryResource in rapidsai#2284.

Rework the binning test so allocations actually span multiple bins.
The previous bin range (2^18-2^22) pre-allocated ~992 MiB of managed
memory while test data never exceeded 1 KiB, so every allocation
landed in the same bin.  Shrink the auto-created range to
2^10-2^17 (1 KiB-128 KiB) so the six _BINNING_NELEMS values each
route to a distinct fixed-size bin with float64.  Add an explicit
128 MiB CudaMemoryResource bin and a dedicated test_binning_large_allocation
that copies 128 MiB of data through it.
rapids-bot bot pushed a commit that referenced this pull request Mar 16, 2026
…tests (#2291)

Scope the `FixedSizeMemoryResource` fixture to the test class so the resource is created once per upstream rather than once per `(dtype, nelem, alloc)` combination, matching the pattern from #2284.

Rework `test_binning_memory_resource` so allocations span multiple distinct bins. The previous bin range `(2^18–2^22)` pre-allocated ~992 MiB of managed memory while test data never exceeded 1 KiB — every allocation landed in the same bin. The new range `(2^10–2^17)` creates bins from 1 KiB to 128 KiB, and each `_BINNING_NELEMS` value routes to a different fixed-size bin with `float64`. An explicit 128 MiB `CudaMemoryResource` bin and a dedicated `test_binning_large_allocation` exercise the large-allocation path.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Tom Augspurger (https://github.com/TomAugspurger)

URL: #2291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function non-breaking Non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants