Skip to content

refactor: Remove memory size elements from BatchSolverConfig, use buffer_registry pattern#517

Merged
ccam80 merged 5 commits into
mainfrom
copilot/refactor-memory-management
Jan 20, 2026
Merged

refactor: Remove memory size elements from BatchSolverConfig, use buffer_registry pattern#517
ccam80 merged 5 commits into
mainfrom
copilot/refactor-memory-management

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 20, 2026

Memory allocation tracking in BatchSolverKernel was anti-pattern—storing sizes in BatchSolverConfig rather than querying buffer_registry like other CUDAFactory subclasses.

Changes

BatchSolverConfig

  • Removed local_memory_elements and shared_memory_elements fields

BatchSolverKernel

  • Properties now delegate to SingleIntegratorRun (which queries buffer_registry):
    @property
    def local_memory_elements(self) -> int:
        return self.single_integrator.local_memory_elements
    
    @property
    def shared_memory_bytes(self) -> int:
        return self.single_integrator.shared_memory_bytes
  • __init__ — removed memory elements from config instantiation
  • run() — removed memory elements from update_compile_settings
  • build_kernel() — uses self.shared_memory_elements property instead of config
  • update() — added buffer_registry.update() call to recognize buffer location parameters

Tests

  • Removed assertions for deleted config fields
Original prompt

Run the pipeline on this task. As the orchestrating agent, do not gather context, read, or edit files. The usage of shared and local memory elements/bytes in singleintegratorrun and batchsolverkernel is anti-pattern. CUDAFactory already features methods to get allocation elements from buffer_registry. BatchSolverKernel should do the same. Remove memory size elements from bachsolverconfig, remove custom properties for loop memory sizes from singleintegratorrun, add the shared_memory_bytes logic to a batchsolverkernel property instead of having it on batchsolverkernel. This will require you to use get_child_allocators in BatchSolverKernel to get array sizes, and to update buffer registry in batchsolverkernel.update. You may need to modify buffer_registry slightly to accomodate the extra "padding" element.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 20, 2026 04:45
…tchSolverKernel to query buffer_registry

Co-authored-by: ccam80 <63381855+ccam80@users.noreply.github.com>
Co-authored-by: ccam80 <63381855+ccam80@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor memory management in batch solver kernel refactor: Remove memory size elements from BatchSolverConfig, use buffer_registry pattern Jan 20, 2026
Copilot AI requested a review from ccam80 January 20, 2026 05:05
@ccam80
Copy link
Copy Markdown
Owner

ccam80 commented Jan 20, 2026

@copilot The pattern in the repo is to delegate shared and local sizes to buffer registry. Remove all comments that highlight this behaviour. Remove all tests which test that the settings have changed/arent there. If the tests for existing functionality dont fail.then the change was successful. Remove ai scaffold.docs in active_plans. The whole change should be the removal of attrivutes, updates, and properties, with very few additions and edits.

…nts per review

Co-authored-by: ccam80 <63381855+ccam80@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jan 20, 2026

@copilot The pattern in the repo is to delegate shared and local sizes to buffer registry. Remove all comments that highlight this behaviour. Remove all tests which test that the settings have changed...

Done in 728037f. Removed all comments highlighting delegation behavior, removed all tests for the new pattern, removed AI scaffold docs in active_plans, and removed the new integration test file. The change is now minimal: 53 lines removed, 7 lines added across 4 files.

@ccam80 ccam80 marked this pull request as ready for review January 20, 2026 09:13
Copilot AI review requested due to automatic review settings January 20, 2026 09:13
@ccam80 ccam80 merged commit 491429c into main Jan 20, 2026
9 checks passed
@ccam80 ccam80 deleted the copilot/refactor-memory-management branch January 20, 2026 09:16
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors memory allocation tracking in BatchSolverKernel to follow the established buffer_registry pattern used by other CUDAFactory subclasses, eliminating the anti-pattern of storing memory sizes directly in BatchSolverConfig.

Changes:

  • Removed local_memory_elements and shared_memory_elements fields from BatchSolverConfig
  • Updated BatchSolverKernel to query memory sizes from SingleIntegratorRun properties (which delegate to buffer_registry) instead of storing them in compile settings
  • Added buffer_registry.update() call in BatchSolverKernel.update() to support buffer location parameter updates

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/cubie/batchsolving/BatchSolverConfig.py Removed local_memory_elements and shared_memory_elements field definitions and documentation
src/cubie/batchsolving/BatchSolverKernel.py Refactored to use property delegation pattern: removed memory elements from config initialization and updates; properties now delegate to single_integrator; added buffer_registry.update() call
tests/batchsolving/test_config_plumbing.py Removed assertion checking for deleted compile_settings.local_memory_elements field
tests/batchsolving/test_SolverKernel.py Removed assertion checking for deleted compile_settings.local_memory_elements field

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jan 20, 2026

Greptile Summary

This PR refactors memory allocation tracking in BatchSolverKernel to follow the buffer_registry pattern used by other CUDAFactory subclasses, eliminating the anti-pattern of storing memory sizes directly in BatchSolverConfig.

Key changes:

  • Removed local_memory_elements and shared_memory_elements fields from BatchSolverConfig
  • Modified BatchSolverKernel properties to delegate to SingleIntegratorRun (which queries buffer_registry via self._loop)
  • Added buffer_registry.update() call in BatchSolverKernel.update() to recognize buffer location parameters
  • Updated build_kernel() to use self.shared_memory_elements property instead of config.shared_memory_elements
  • Removed redundant memory element assignments from __init__() and run()
  • Updated tests to remove assertions for deleted config fields

The refactoring successfully aligns BatchSolverKernel with the established CUDAFactory memory management pattern.

Confidence Score: 5/5

  • This PR is safe to merge - it's a clean architectural refactoring with proper test updates
  • The refactoring follows established patterns in the codebase, removes an architectural anti-pattern, and properly updates all affected code paths including tests. The changes maintain backward compatibility by preserving the public API via properties while changing the underlying implementation to query buffer_registry.
  • No files require special attention

Important Files Changed

Filename Overview
src/cubie/batchsolving/BatchSolverConfig.py Removed local_memory_elements and shared_memory_elements fields from config class, simplifying the data model
src/cubie/batchsolving/BatchSolverKernel.py Refactored to query buffer_registry via properties instead of storing memory sizes in config; added buffer_registry.update() call in update() method

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.

3 participants