fix: gdsfactory 9.43 compatibility (duplicate port + deprecated params)#165
Open
joamatab wants to merge 1 commit into
Open
fix: gdsfactory 9.43 compatibility (duplicate port + deprecated params)#165joamatab wants to merge 1 commit into
joamatab wants to merge 1 commit into
Conversation
- Patch _register_ports to skip position-equality duplicate check. IHP transistors have pin-marker boxes on different layers (Metal1, GatPoly) at overlapping positions. kfactory BasePort.__eq__ only compares transforms, causing false "duplicate port" errors. - Remove deprecated component_name and taper params from add_pads_top (removed in gdsfactory 9.43 route_bundle API).
Contributor
Reviewer's GuideAdds a compatibility patch for gdsfactory 9.43’s port registration behavior and removes deprecated parameters from IHP pad-adding container helpers to match the new routing API. Sequence diagram for patched gdsfactory _register_ports behaviorsequenceDiagram
participant Caller
participant gdsfactory_add_ports as gdsfactory_add_ports
participant _name_only_register_ports as _name_only_register_ports
participant Component as component
Caller->>gdsfactory_add_ports: _register_ports(component, ports, auto_rename_ports, allow_none_names)
activate gdsfactory_add_ports
gdsfactory_add_ports->>_name_only_register_ports: _name_only_register_ports(component, ports, auto_rename_ports, allow_none_names)
activate _name_only_register_ports
_name_only_register_ports->>gdsfactory_add_ports: sort_ports_clockwise(ports)
gdsfactory_add_ports-->>_name_only_register_ports: ports (sorted)
loop for each port in ports
_name_only_register_ports->>Component: check port.name in component.ports
alt name already in component.ports
_name_only_register_ports-->>Caller: ValueError
else name not in component.ports
_name_only_register_ports->>Component: add_port(name=port.name, port=port)
end
end
alt auto_rename_ports
_name_only_register_ports->>Component: auto_rename_ports()
end
_name_only_register_ports-->>gdsfactory_add_ports: component
deactivate _name_only_register_ports
gdsfactory_add_ports-->>Caller: component
deactivate gdsfactory_add_ports
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}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The global monkeypatch of
_gf_add_ports._register_portsinutils.pyaffects all gdsfactory usage; consider scoping the workaround to just the IHP flow (e.g., via a local helper that calls the patched logic or by patching/unpatching around specific calls) to avoid surprising behavior elsewhere. - It may be safer to guard the
_register_portsoverride on the detected gdsfactory version and/or feature-test the original behavior, so that future gdsfactory changes don’t silently interact badly with this compatibility shim.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The global monkeypatch of `_gf_add_ports._register_ports` in `utils.py` affects all gdsfactory usage; consider scoping the workaround to just the IHP flow (e.g., via a local helper that calls the patched logic or by patching/unpatching around specific calls) to avoid surprising behavior elsewhere.
- It may be safer to guard the `_register_ports` override on the detected gdsfactory version and/or feature-test the original behavior, so that future gdsfactory changes don’t silently interact badly with this compatibility shim.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the add_pads_top function across the ihp cells by removing the component_name and taper parameters. It also implements a monkey-patch in ihp/cells2/utils.py to override gdsfactory port registration behavior, preventing false rejections of overlapping ports. Feedback indicates that the logic for checking existing port names is flawed and inefficient, and it suggests moving the monkey-patch to a central initialization location to ensure it is reliably applied.
Comment on lines
+24
to
+34
| for port in ports: | ||
| _port_name = port.name | ||
| if allow_none_names and _port_name is None: | ||
| continue | ||
| if _port_name is not None and _port_name in component.ports: | ||
| component_ports = [p.name for p in component.ports] | ||
| raise ValueError( | ||
| f"port {_port_name!r} already in {component_ports}. " | ||
| "You can pass a port_name_prefix to add it with a different name." | ||
| ) | ||
| component.add_port(name=_port_name, port=port) |
There was a problem hiding this comment.
The collision check _port_name in component.ports is likely incorrect for gdsfactory 9.x/kfactory. In these versions, component.ports is a PortStack (which behaves like a list of Port objects), so checking if a string (_port_name) is in the stack will always return False. This prevents the custom ValueError from ever being raised, falling back to the default (and potentially less descriptive) error from add_port.
Additionally, performing this check inside the loop results in O(N*M) complexity. A more efficient and correct approach would be to pre-calculate the set of existing port names.
Suggested change
| for port in ports: | |
| _port_name = port.name | |
| if allow_none_names and _port_name is None: | |
| continue | |
| if _port_name is not None and _port_name in component.ports: | |
| component_ports = [p.name for p in component.ports] | |
| raise ValueError( | |
| f"port {_port_name!r} already in {component_ports}. " | |
| "You can pass a port_name_prefix to add it with a different name." | |
| ) | |
| component.add_port(name=_port_name, port=port) | |
| existing_names = {p.name for p in component.ports} | |
| for port in ports: | |
| _port_name = port.name | |
| if allow_none_names and _port_name is None: | |
| continue | |
| if _port_name is not None and _port_name in existing_names: | |
| raise ValueError( | |
| f"port {_port_name!r} already in {list(existing_names)}. " | |
| "You can pass a port_name_prefix to add it with a different name." | |
| ) | |
| component.add_port(name=_port_name, port=port) | |
| existing_names.add(_port_name) |
| return component | ||
|
|
||
|
|
||
| _gf_add_ports._register_ports = _name_only_register_ports |
There was a problem hiding this comment.
Applying a global monkey-patch at the module level of a utility file can be fragile. If this module is not imported before a component is instantiated or a routing function is called, the patch will not be active. Consider moving this workaround to a more central initialization location (e.g., the package's __init__.py) to ensure it is consistently applied across the PDK, especially since ihp/cells/containers.py does not explicitly import this utility.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
_register_portsto skip position-equality duplicate check — IHP transistors have pin-marker boxes on different layers (Metal1, GatPoly) at overlapping positions, and kfactoryBasePort.__eq__only compares transforms, causing false "duplicate port" errorscomponent_nameandtaperparams fromadd_pads_top(removed in gdsfactory 9.43route_bundleAPI)Test plan
make test-force)Summary by Sourcery
Ensure compatibility with gdsfactory 9.43 by adjusting port registration and updating pad-routing helpers.
Bug Fixes:
Enhancements: