Canonicalize liquid_handler code #178
Merged
Conversation
This file contains hidden or 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
修改移液枪的状态判断方法， 添加三轴的表定点与零点之间的转换 添加三轴真实移动的backend
简化移动方法， 取消软件限制位置， 修改当值使用Z轴时也需要重新复位Z轴的问题
1,现在可以直接通过修改backend，适配其他的移液站，主类依旧使用LiquidHandler，不用重新编写 2,修改枪头判断标准，使用枪头自身判断而不是类的判断， 3,将归零参数用毫米计算，方便手动调整， 4,修改归零方式，上电使用机械归零，确定机械零点，手动归零设置工作区域零点方便计算，二者互不干涉
Reviewer's GuideExtends liquid handling device flexibility and visualization by allowing dict-configured backends, enriching deck and resource metadata (including 3D model paths and slot layout), changing mix_times from scalar to array in multiple device schemas, and tightening category/pose handling for PRCXI and Laiyu resources. Sequence diagram for dict-configured backend resolution in LiquidHandlerAbstractsequenceDiagram
participant Caller
participant LiquidHandlerAbstract
participant Globals
participant Importlib
participant Pylabrobot
participant SuperMiddleware as LiquidHandlerMiddleware
Caller->>LiquidHandlerAbstract: __init__(backend=dict_with_type, deck, simulator, channel_num, backend_kwargs)
activate LiquidHandlerAbstract
LiquidHandlerAbstract->>LiquidHandlerAbstract: check isinstance(backend, dict) and has key type
alt type in globals
LiquidHandlerAbstract->>Globals: lookup backend_cls = globals[type_str]
else dotted notation
LiquidHandlerAbstract->>Importlib: import_module(module_name)
Importlib-->>LiquidHandlerAbstract: module or error
LiquidHandlerAbstract->>Importlib: getattr(mod, class_name)
else pylabrobot namespace
LiquidHandlerAbstract->>Pylabrobot: getattr(pylabrobot, type_str)
Pylabrobot-->>LiquidHandlerAbstract: backend_cls or None
end
alt backend_cls is valid type
LiquidHandlerAbstract->>LiquidHandlerAbstract: backend_type = backend_cls(**backend_dict)
else failure
LiquidHandlerAbstract->>Caller: raise RuntimeError
end
LiquidHandlerAbstract->>LiquidHandlerMiddleware: super().__init__(backend_type, deck, simulator, channel_num, backend_kwargs)
deactivate LiquidHandlerAbstract
Class diagram for updated LiquidHandlerAbstract backend initializationclassDiagram
class LiquidHandlerBackend
class Deck
class LiquidHandlerMiddleware {
<<abstract>>
}
class LiquidHandlerAbstract {
+bool support_touch_tip
+BaseROS2DeviceNode _ros_node
-bool _simulator
-dict group_info
+LiquidHandlerAbstract(backend, deck, simulator, channel_num, backend_kwargs)
+post_init(ros_node)
}
LiquidHandlerAbstract --|> LiquidHandlerMiddleware
LiquidHandlerAbstract --> LiquidHandlerBackend : uses
LiquidHandlerAbstract --> Deck : uses
class LiquidHandlerAbstractInitFlow {
-backend_type
+resolve_backend(backend)
}
LiquidHandlerAbstract ..> LiquidHandlerAbstractInitFlow : initialization_logic
Class diagram for updated PRCXI9300Deck with slot_locations and serializeclassDiagram
class Coordinate {
+float x
+float y
+float z
}
class Deck {
+string name
+float size_x
+float size_y
+float size_z
+serialize()
}
class PRCXI9300Deck {
+list slots
+list slot_locations
+PRCXI9300Deck(name, size_x, size_y, size_z, kwargs)
+serialize()
}
PRCXI9300Deck --|> Deck
PRCXI9300Deck "12" o-- Coordinate : slot_locations
class SiteSerializationItem {
+string label
+bool visible
+position_x
+position_y
+position_z
+size_width
+size_height
+size_depth
+list content_type
}
PRCXI9300Deck ..> SiteSerializationItem : builds_sites_in_serialize
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
resource_container.yamlandlaiyu/deck.yamlyou now have duplicate top‑level keys (tiprack_box,TransformXYZDeck) defined twice, which in YAML will silently override one definition; please consolidate each into a single entry to avoid confusing or unintended behavior. - The new
get_resource_instance_from_dictlogic inresource_tracker.pyassumescontent["config"]always exists and only setsposewhen"position" in content; consider usingcontent.get("config", {})and preserving the previous fallback behavior whenpositionis missing to avoidKeyErrorand missingposein some inputs. - In
PRCXI9300Deck.serializethere areprintdebug statements that will run on every serialization; these should be removed or replaced with proper logging to avoid noisy stdout in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resource_container.yaml` and `laiyu/deck.yaml` you now have duplicate top‑level keys (`tiprack_box`, `TransformXYZDeck`) defined twice, which in YAML will silently override one definition; please consolidate each into a single entry to avoid confusing or unintended behavior.
- The new `get_resource_instance_from_dict` logic in `resource_tracker.py` assumes `content["config"]` always exists and only sets `pose` when `"position" in content`; consider using `content.get("config", {})` and preserving the previous fallback behavior when `position` is missing to avoid `KeyError` and missing `pose` in some inputs.
- In `PRCXI9300Deck.serialize` there are `print` debug statements that will run on every serialization; these should be removed or replaced with proper logging to avoid noisy stdout in production.
## Individual Comments
### Comment 1
<location> `unilabos/registry/resources/common/resource_container.yaml:236-234` </location>
<code_context>
registry_type: resource
version: 1.0.0
+
+tiprack_box:
+ category:
+ - resource_container
+ class:
+ module: unilabos.devices.resource_container.container:TipRackContainer
+ type: python
+ description: 96针头盒
+ handles: []
+ icon: ''
+ init_param_schema: {}
+ model:
+ children_mesh: tip/meshes/tip.stl
+ children_mesh_path: https://uni-lab.oss-cn-zhangjiakou.aliyuncs.com/uni-lab/resources/tip/modal.xacro
+ children_mesh_tf:
+ - 0.0045
+ - 0.0045
+ - 0
+ - 0
+ - 0
+ - 0
+ mesh: tiprack_box/meshes/tiprack_box.stl
+ path: https://uni-lab.oss-cn-zhangjiakou.aliyuncs.com/uni-lab/resources/tiprack_box/modal.xacro
+ mesh_tf:
+ - 0
+ - 0
+ - 0
+ - 0
+ - 0
+ - 0
+ type: resource
+ registry_type: resource
+ version: 1.0.0
+
+plate_96:
</code_context>
<issue_to_address>
**issue (bug_risk):** The `tiprack_box` entry is defined twice, which in YAML means the earlier definition is silently overwritten.
Because YAML keeps only the last value for a given key, this new `tiprack_box` block will replace the earlier one entirely. Please confirm whether you meant to fully replace it; otherwise, consider renaming this resource or merging these fields into the original definition so existing configuration isn’t lost.
</issue_to_address>
### Comment 2
<location> `unilabos/devices/liquid_handling/prcxi/prcxi.py:98-100` </location>
<code_context>
+ "content_type": ["plate", "tip_rack", "tube_rack"],
+ } for i, slot in enumerate(self.slot_locations)
+ ]
+ print("----"*10)
+ print(super_serialized)
+ return super_serialized
class PRCXI9300Container(Plate, TipRack):
</code_context>
<issue_to_address>
**issue (performance):** Debug `print` statements in `serialize()` will produce unwanted console output in production.
Since `serialize()` is likely called often, these `print` calls will spam stdout and add unnecessary overhead. Please remove them or switch to the project’s logger at an appropriate level (e.g. `debug`).
</issue_to_address>
### Comment 3
<location> `unilabos/ros/nodes/resource_tracker.py:149-158` </location>
<code_context>
- if "pose" not in content:
- content["pose"] = content.get("position", {})
+
+ if "position" in content:
+ pose = content["config"].get("pose",{})
+ if "position" not in pose :
+ if "position" in content["position"]:
+ pose["position"] = content["position"]["position"]
+ else:
+ pose["position"] = {"x": 0, "y": 0, "z": 0}
+ if "size" not in pose:
+ pose["size"] = {
+ "width": content["config"].get("size_x", 0),
+ "height": content["config"].get("size_y", 0),
+ "depth": content["config"].get("size_z", 0)
+ }
+ content["pose"] = pose
return ResourceDictInstance(ResourceDict.model_validate(content))
</code_context>
<issue_to_address>
**issue (bug_risk):** New pose construction logic assumes `content["config"]` always exists, which can raise a `KeyError`.
Here `pose = content["config"].get("pose", {})` will raise if `"config"` is absent. The previous logic didn’t depend on `config`, so this is a potential regression. Use `content.get("config", {})` (and similar for other `config` reads) or guard this block with a `"config" in content` check.
</issue_to_address>
### Comment 4
<location> `unilabos/registry/resources/laiyu/deck.yaml:18-16` </location>
<code_context>
registry_type: resource
version: 1.0.0
+
+TransformXYZDeck:
+ category:
+ - deck
+ class:
+ module: unilabos.devices.liquid_handling.laiyu.laiyu:TransformXYZDeck
+ type: pylabrobot
+ description: Laiyu deck
+ handles: []
+ icon: ''
+ init_param_schema: {}
+ model:
+ mesh: liquid_transform_xyz
+ path: https://uni-lab.oss-cn-zhangjiakou.aliyuncs.com/uni-lab/devices/liquid_transform_xyz/macro_device.xacro
+ type: device
+ registry_type: resource
+ version: 1.0.0
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The `TransformXYZDeck` resource is declared twice, so the second definition will override the first.
Because YAML only keeps the last occurrence of a duplicate top-level key, the earlier `TransformXYZDeck` definition will be silently discarded. If you meant to extend or modify the existing resource, please merge these fields into the original block rather than redefining the key, so you don’t lose any configuration from the first definition.
</issue_to_address>
### Comment 5
<location> `unilabos/devices/liquid_handling/liquid_handler_abstract.py:592` </location>
<code_context>
def __init__(self, backend: LiquidHandlerBackend, deck: Deck, simulator: bool=False, channel_num:int = 8,**backend_kwargs):
"""Initialize a LiquidHandler.
Args:
backend: Backend to use.
deck: Deck to use.
"""
backend_type = None
if isinstance(backend, dict) and "type" in backend:
backend_dict = backend.copy()
type_str = backend_dict.pop("type")
try:
# Try to get class from string using globals (current module), or fallback to pylabrobot or unilabos namespaces
backend_cls = None
if type_str in globals():
backend_cls = globals()[type_str]
else:
# Try resolving dotted notation, e.g. "xxx.yyy.ClassName"
components = type_str.split(".")
mod = None
if len(components) > 1:
module_name = ".".join(components[:-1])
try:
import importlib
mod = importlib.import_module(module_name)
except ImportError:
mod = None
if mod is not None:
backend_cls = getattr(mod, components[-1], None)
if backend_cls is None:
# Try pylabrobot style import (if available)
try:
import pylabrobot
backend_cls = getattr(pylabrobot, type_str, None)
except Exception:
backend_cls = None
if backend_cls is not None and isinstance(backend_cls, type):
backend_type = backend_cls(**backend_dict) # pass the rest of dict as kwargs
except Exception as exc:
raise RuntimeError(f"Failed to convert backend type '{type_str}' to class: {exc}")
else:
backend_type = backend
self._simulator = simulator
self.group_info = dict()
super().__init__(backend_type, deck, simulator, channel_num,**backend_kwargs)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
- Low code quality found in LiquidHandlerAbstract.\_\_init\_\_ - 24% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
```suggestion
raise RuntimeError(
f"Failed to convert backend type '{type_str}' to class: {exc}"
) from exc
```
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| path: https://uni-lab.oss-cn-zhangjiakou.aliyuncs.com/uni-lab/resources/tiprack_box/modal.xacro | ||
| type: resource | ||
| registry_type: resource | ||
| version: 1.0.0 |
There was a problem hiding this comment.
issue (bug_risk): The tiprack_box entry is defined twice, which in YAML means the earlier definition is silently overwritten.
Because YAML keeps only the last value for a given key, this new tiprack_box block will replace the earlier one entirely. Please confirm whether you meant to fully replace it; otherwise, consider renaming this resource or merging these fields into the original definition so existing configuration isn’t lost.
| path: https://uni-lab.oss-cn-zhangjiakou.aliyuncs.com/uni-lab/devices/liquid_transform_xyz/macro_device.xacro | ||
| type: device | ||
| registry_type: resource | ||
| version: 1.0.0 |
There was a problem hiding this comment.
issue (bug_risk): The TransformXYZDeck resource is declared twice, so the second definition will override the first.
Because YAML only keeps the last occurrence of a duplicate top-level key, the earlier TransformXYZDeck definition will be silently discarded. If you meant to extend or modify the existing resource, please merge these fields into the original block rather than redefining the key, so you don’t lose any configuration from the first definition.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
修改上传大小方法
Summary by Sourcery
Update liquid handling backends, deck definitions, and resource metadata to support richer configuration, visualization, and result reporting.
New Features:
Bug Fixes:
Enhancements: