Skip to content

Dynamically assign device_type based on subclass name#68

Open
lochhh wants to merge 2 commits intomainfrom
auto-device-type
Open

Dynamically assign device_type based on subclass name#68
lochhh wants to merge 2 commits intomainfrom
auto-device-type

Conversation

@lochhh
Copy link
Member

@lochhh lochhh commented Feb 5, 2026

addresses #67 ?

I updated device_type in the test Metadata.json to match the subclasses defined in test_base.py (is this correct?)

@lochhh lochhh requested a review from glopesdev February 5, 2026 19:42
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.69%. Comparing base (1ac385b) to head (a9f06fd).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
- Coverage   78.91%   78.69%   -0.22%     
==========================================
  Files          15       15              
  Lines         792      784       -8     
==========================================
- Hits          625      617       -8     
  Misses        167      167              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lochhh lochhh linked an issue Feb 6, 2026 that may be closed by this pull request
@glopesdev
Copy link
Contributor

@lochhh This solution works really well, thanks!

I tested it with a couple of project-specific schemas and it worked great there too, but it did raise a few questions I am not entirely sure about, specifically about local extensions. Consider the following:

class TriggerName(StrEnum):
    TRIGGER0 = "Trigger0"
    TRIGGER1 = "Trigger1"

class CameraControllerTrigger(BaseSchema):
    frequency: int = Field(
        default=50,
        description="The frequency of the camera TTL trigger.",
    )

class CameraController(HarpCameraControllerGen2):
    triggers: Dict[TriggerName,CameraControllerTrigger]

This local CameraController class is really just an extension of the Harp camera controller to include the specific triggers we use on the SWC rigs, but I wonder if we want to hide that the device type is really the HarpCameraControllerGen2.

This may be the case, but it did leave me pondering. I do think we should still go ahead with this change, even at the risk of potential name clashes in the odd case.

Anyway, here is how the above would render previously:

    "CameraController": {
      "properties": {
        "deviceType": {
          "const": "HarpCameraControllerGen2",
          "default": "HarpCameraControllerGen2",
          "title": "DeviceType",
          "type": "string"
        },

Snapshot of previous rendering for the UndergroundFeeder for reference:

    "UndergroundFeeder": {
      "description": "Represents control and acquisition functionality for an underground feeder module.",
      "properties": {
        "deviceType": {
          "const": "HarpOutputExpander",
          "default": "HarpOutputExpander",
          "title": "DeviceType",
          "type": "string"
        },

@glopesdev
Copy link
Contributor

The other question this raised for me is how to deal with serialization when having a collection where the device type is not needed as a discriminator, such as a list of cameras. In that case the serialization to JSON ends up emitting deviceType where it is really redundant, but maybe this is something I can figure out on the JSON serializer side. Again just leaving here for reference.

@glopesdev
Copy link
Contributor

@lochhh another disadvantage we realized of this solution is we lose static typing, as the field will be typed as Any in sub-classes.

That combined with my above comment is making me wonder that perhaps we don't want to force the discriminator at the level of base classes, but only as necessary in specific collections requiring a discriminator.

Since we allow declaration of project-specific classes and pydantic will resolve specific types when parsing JSON / YAML metadata anyway, such field types would only really be used when dealing with true polymorphic collections.

Therefore my inclination is now to simply remove these literals from all base classes entirely, I will make a issue / PR with a proposal for this that we can discuss better next time.

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.

UndergroundFeeder.device_type is set as HarpOutputExpander

3 participants