Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 2, 2025

Migrate converters to metaclass-registry system ✅ COMPLETE

Summary:
Successfully migrated arraybridge converters to use metaclass-registry for automatic registration and discovery. All converters now auto-register via the AutoRegisterMeta metaclass with a clean implementation - no backward compatibility bloat.

Key Achievements:

  • ✅ 100% clean metaclass-registry implementation (removed all backward compatibility layers)
  • ✅ Deleted conversion_helpers.py entirely (~113 lines removed)
  • ✅ All 6 memory types auto-register on module import
  • ✅ Registry auto-validates configuration at import time
  • ✅ All existing tests pass (verified manually)
  • ✅ CodeQL security scan: Clean (0 issues)
  • ✅ Comprehensive documentation for developers

Recent Updates:

  • Removed unnecessary version constraint from metaclass-registry dependency
  • Removed conversion_helpers.py entirely - no backward compatibility bloat
  • Removed backward compatibility test from test_registry_integration.py
  • Updated documentation to reflect clean implementation

Files Changed:

  • pyproject.toml: Added metaclass-registry dependency (no version constraint)
  • src/arraybridge/converters_registry.py: New registry infrastructure (168 lines)
  • src/arraybridge/conversion_helpers.py: DELETED (was backward compatibility layer)
  • src/arraybridge/types.py: Updated converter property (1 line change)
  • src/arraybridge/converters.py: Updated to use registry (-2 lines)
  • tests/test_converters_registry.py: Registry tests (109 lines)
  • tests/test_registry_integration.py: Integration tests (130 lines, removed backward compat test)
  • ADDING_NEW_FRAMEWORKS.md: Developer guide (updated to reflect clean implementation)
  • MIGRATION_SUMMARY.md: Complete migration summary (updated to reflect clean implementation)

Clean Implementation Benefits:

  1. No bloat: Removed all backward compatibility layers
  2. Auto-registration: New frameworks only need enum + config
  3. Discoverability: Registry provides programmatic access
  4. Validation: Auto-validates at import time
  5. Pure metaclass-registry: Clean implementation following best practices
Original prompt

GitHub Copilot Chat Assistant

Short answer

  • Yes — using metaclass-registry is a good fit. It will remove manual wiring, make adding new frameworks simpler, and maps naturally to the current pattern where each memory framework has a converter implementation.
  • Because you said backwards compatibility isn’t required, prefer a clean migration: make converters auto-register and use the registry at runtime (instead of keeping a manual _CONVERTERS dict).

Why it fits (based on the repos)

  • metaclass-registry exposes AutoRegisterMeta and a convention where classes define a registry key via a class attribute (registry_key / per-docs) and are auto-added to BaseClass.registry.
  • arraybridge currently has an enum MemoryType plus a module-level _CONVERTERS mapping used by MemoryType.converter. That mapping is exactly the thing you can replace with an auto-registering converter class registry.

Recommended integration pattern

  1. Create a Converter base class that uses the metaclass:

    • class ConverterBase(metaclass=AutoRegisterMeta):
      registry_key = "memory_type"
      memory_type: str = None
      def to_numpy(...): ...

      optional: factory/instance method

  2. Implement each framework converter as a subclass that sets memory_type to the same string used in MemoryType (e.g., "numpy", "torch"). Those classes will auto-register into ConverterBase.registry.

  3. Replace _CONVERTERS lookups with registry lookups:

    • a simple accessor: get_converter(mem_type_str) -> converter class/instance = ConverterBase.registry[mem_type_str]
    • update MemoryType.converter property to use get_converter(self.value).
  4. Ensure discovery/imports:

    • Either ensure every converter module is imported at package init (explicit imports) or use metaclass-registry’s discovery utilities (discover_registry_classes / LazyDiscoveryDict) to lazily discover classes inside a package. This avoids “missing registration because module wasn’t imported” problems.

Concrete example (conceptual)

  • Base:
    class ConverterBase(metaclass=AutoRegisterMeta):
    registry_key = "memory_type"
    memory_type: str = None
    def to_numpy(self, data, gpu_id): raise NotImplementedError
    # optional factory
  • Concrete:
    class NumpyConverter(ConverterBase):
    memory_type = "numpy"
    def to_numpy(self, data, gpu_id): return data # no-op
  • Lookup:
    def get_converter(mem_type_str):
    cls = ConverterBase.registry.get(mem_type_str)
    if cls is None: raise ValueError(...)
    return cls() # or return cls if you prefer singleton classes
  • Update MemoryType.converter:
    @Property
    def converter(self):
    return get_converter(self.value)

Migration steps (practical)

  1. Add new module src/arraybridge/converters_registry.py with ConverterBase (AutoRegisterMeta), get_converter() helper, and any discovery logic.
  2. Convert conversion implementations in conversion_helpers to subclasses of ConverterBase (one-by-one).
  3. Replace existing _CONVERTERS uses:
    • MemoryType.converter -> get_converter(...)
    • convert_memory/detect_memory_type -> use registry-based converter instances
  4. Run tests; add small tests asserting ConverterBase.registry contains expected keys, and that MemoryType.converter works.
  5. Optionally remove the old _CONVERTERS mapping after migration completes.

Benefits you’ll get

  • New frameworks are added by creating a class file — no central dict changes.
  • Cleaner per-framework encapsulation: each converter class holds its ops.
  • Can use metaclass-registry advanced features later:
    • key extractors (if your class names follow a convention)
    • secondary registries (e.g., separate ops registry)
    • lazy discovery & caching

Pitfalls and how to avoid them

  • Missing imports (types not registered): address by either
    • explicit package imports in init.py, or
    • using discover_registry_classes to import modules under a package prefix when registry is first accessed.
  • Naming/key stability: use the same stable string keys as MemoryType.value to avoid breaking serialized data.
  • Circular imports: define the registry base in a very low-level module (e.g., arraybridge.converters_registry) that other modules can import without pulling in heavy modules.
  • Tests: add tests that assert registration happens (so CI fails early if discovery/importing breaks).

Extras to consider (from metaclass-registry features)

  • make_suffix_extractor can derive keys from class names if you prefer not to set memory_type explicitly.
  • SecondaryRegistry could be useful if you need to expose a separate mapping (for example, a mapping of “ops” or “availability checks”) produced automatically from the primary registry.
  • RegistryCacheManager and LazyDiscoveryDict are available if startup cost or discovery caching becomes important.

Tests to add

  • Test that ConverterBase.registry contains keys for each MemoryType.value after package import/discovery.
  • Test MemoryType.converter returns a working con...

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI self-assigned this Nov 2, 2025
Copilot AI and others added 6 commits November 2, 2025 13:21
…sed converters

Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
…urity documentation

Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement metaclass-registry for automatic converter registration Migrate converters to metaclass-registry for auto-registration Nov 2, 2025
Copilot AI requested a review from trissim November 2, 2025 13:36
@trissim trissim marked this pull request as ready for review November 2, 2025 17:05
@trissim
Copy link
Owner

trissim commented Nov 2, 2025

@copilot why is he minmum version of metaclass-regitry 0.4.0? doesn't even exist. don't specify a minimum version unecessarily

Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 2, 2025

@copilot why is he minmum version of metaclass-regitry 0.4.0? doesn't even exist. don't specify a minimum version unecessarily

Fixed in 7e12d7b. Removed the version constraint - the dependency is now specified as just "metaclass-registry" without a minimum version since only 0.1.0 exists on PyPI.

@trissim
Copy link
Owner

trissim commented Nov 2, 2025

@copilot I see mention of backwards compatibility layers. Get rid of them and make eveyrhting usgin the metaclass-regsitry pattern properly. no bloat

…entation

Co-authored-by: trissim <56880052+trissim@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 2, 2025

@copilot I see mention of backwards compatibility layers. Get rid of them and make eveyrhting usgin the metaclass-regsitry pattern properly. no bloat

Done in cbf3253. Removed all backward compatibility layers:

  • Deleted conversion_helpers.py entirely (~113 lines removed)
  • Removed backward compatibility test from test_registry_integration.py
  • Updated documentation to reflect clean implementation

Now using pure metaclass-registry pattern with no bloat. All converters auto-register directly via ConverterBase.__registry__ with zero backward compatibility code.

@trissim trissim merged commit ddc9973 into main Nov 2, 2025
18 of 28 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.

2 participants