chore(api): allow specifying module models to sim #15104
Merged
Conversation
This file contains 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
The simulator setup files let you specify modules, but you can only do it by specifying their types (i.e. magdeck, tempdeck) and not their modules (i.e. magneticModuleV1, temperatureModuleV2). This hasn't really mattered up til now because nothing cares, but RSQ-6 will have both desktop and ODD looking pretty hard at which generation of module is present on a flex, so we need a way to specify. This adds a model key to the module section of the simulator setup (and sets the test-flex.json and test.json up with it) so that you can specify module models. Closes RSQ-6
TamarZanzouri
approved these changes
May 7, 2024
| @@ -255,7 +257,7 @@ async def build_hardware_simulator( | |||
| attached_instruments: Optional[ | |||
| Dict[top_types.Mount, Dict[str, Optional[str]]] | |||
| ] = None, | |||
| attached_modules: Optional[Dict[str, List[str]]] = None, | |||
| attached_modules: Optional[Dict[str, List[Tuple[str, Optional[str]]]]] = None, | |||
There was a problem hiding this comment.
Is this the proper data structure for this?
There was a problem hiding this comment.
well, no. i guess this is the point where the dataclasses should get persisted through.
This was getting a bit unwieldy; prior commits had made it a dict of lists of strings, and now this needed it to be a dict of lists of tuples of two strings, and all of this was terrible. Instead now it's a dict of lists of a specific dataclass, much nicer. While I was doing that I noticed that I didn't get any type errors, and that's because ThreadManager's constructor was erasing the types of parameters that you gave it, so I made it use a ParamSpec. Unfortunately, you can't alter wrapped function named arguments using type machinery right now, so I also had to change the way we specify nonblocking thread manager construction to use a wrapper function that sets an attr, but now that's type safe too.
| @@ -255,7 +257,7 @@ async def build_hardware_simulator( | |||
| attached_instruments: Optional[ | |||
| Dict[top_types.Mount, Dict[str, Optional[str]]] | |||
| ] = None, | |||
| attached_modules: Optional[Dict[str, List[str]]] = None, | |||
| attached_modules: Optional[Dict[str, List[modules.SimulatingModule]]] = None, | |||
| `'tempdeck'` or `'magdeck'`) representing | ||
| :param attached_modules: A map of module type names (e.g. | ||
| `'tempdeck'` or `'magdeck'`) to lists of (serial, model) | ||
| tuples representing |
There was a problem hiding this comment.
It's not tuples anymore
| @@ -114,8 +114,6 @@ def synchronizer(*args: Any, **kwargs: Any) -> Any: | |||
| def build_thread_manager() -> ThreadManager[Union[API, OT3API]]: | |||
| return ThreadManager( | |||
| API.build_hardware_controller, | |||
| use_usb_bus=ff.rear_panel_integration(), | |||
There was a problem hiding this comment.
Mmm is this removal ok?
There was a problem hiding this comment.
Yup! This was revealed by ThreadManager.__init__ actually forwarding the types. This code is building an OT2 hardware controller, it doesn't have that argument this code would have crashed (I don't think anybody uses this repl on the OT-2 or indeed the flex anymore, it was made during flex development)
| @@ -38,7 +38,8 @@ | |||
| ] | |||
| HardwareControlAPI = Union[OT2HardwareControlAPI, OT3HardwareControlAPI] | |||
|
|
|||
| ThreadManagedHardware = ThreadManager[HardwareControlAPI] | |||
| # this type ignore is because of https://github.com/python/mypy/issues/13437 | |||
| ThreadManagedHardware = ThreadManager[HardwareControlAPI] # type: ignore[misc] | |||
There was a problem hiding this comment.
Just for me why do we need this type: ignore now? Looks like the code is the same?
There was a problem hiding this comment.
It's this bug in mypy where if you have
- a class that is
Generic - and an
__init__method that takes a typevar
then you get an error when you try to bind to it.
TamarZanzouri
approved these changes
May 7, 2024
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
The simulator setup files let you specify modules, but you can only do it by specifying their types (i.e. magdeck, tempdeck) and not their modules (i.e. magneticModuleV1, temperatureModuleV2). This hasn't really mattered up til now because nothing cares, but RSQ-6 will have both desktop and ODD looking pretty hard at which generation of module is present on a flex, so we need a way to specify. This adds a model key to the module section of the simulator setup (and sets the test-flex.json and test.json up with it) so that you can specify module models. Also, while I was at it, improve the type checking of the init function of the threadmanager. Closes RSQ-6
Carlos-fernandez
pushed a commit
that referenced
this pull request
Jun 3, 2024
The simulator setup files let you specify modules, but you can only do it by specifying their types (i.e. magdeck, tempdeck) and not their modules (i.e. magneticModuleV1, temperatureModuleV2). This hasn't really mattered up til now because nothing cares, but RSQ-6 will have both desktop and ODD looking pretty hard at which generation of module is present on a flex, so we need a way to specify. This adds a model key to the module section of the simulator setup (and sets the test-flex.json and test.json up with it) so that you can specify module models. Also, while I was at it, improve the type checking of the init function of the threadmanager. Closes RSQ-6
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.
The simulator setup files let you specify modules, but you can only do it by specifying their types (i.e. magdeck, tempdeck) and not their modules (i.e. magneticModuleV1, temperatureModuleV2). This hasn't really mattered up til now because nothing cares, but RSQ-6 will have both desktop and ODD looking pretty hard at which generation of module is present on a flex, so we need a way to specify.
This adds a model key to the module section of the simulator setup (and sets the test-flex.json and test.json up with it) so that you can specify module models.
Also, while I was at it, improve the type checking of the init function of the threadmanager.
Closes RSQ-6