Make (gmeep) s-parameter caching more stable#679
Merged
joamatab merged 1 commit intogdsfactory:mainfrom Feb 28, 2026
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts how component GDS files are written and paths handled so that s-parameter caching ignores kfactory/gdsfactory metadata and uses more stable hashing, while also standardizing path handling via pathlib.Path imports. Sequence diagram for stable sparameter caching via GDS hashingsequenceDiagram
actor User
participant Caller as caller_module
participant GetPath as _get_sparameters_path
participant Gf as gf
participant Comp as Component
participant Hash as get_component_hash
participant FS as filesystem
User->>Caller: request_sparameters(component, **kwargs)
Caller->>GetPath: _get_sparameters_path(component, dirpath, **kwargs)
GetPath->>Gf: get_component(component)
Gf-->>GetPath: Component instance
GetPath->>Hash: get_component_hash(component)
activate Hash
Hash->>Comp: write_gds(no_empty_cells=True, with_metadata=False)
Comp-->>Hash: gdspath
Hash->>FS: read_bytes(gdspath)
FS-->>Hash: gds_bytes
Hash->>Hash: md5(gds_bytes)
Hash->>FS: unlink(gdspath)
Hash-->>GetPath: component_hash
deactivate Hash
GetPath->>GetPath: dirpath = Path(dirpath)
GetPath->>GetPath: dirpath = dirpath / component.name
GetPath-->>Caller: sparameters_path
Caller-->>User: return cached_or_computed_sparameters
Flow diagram for get_component_hash using metadatafree GDSflowchart TD
A["get_component_hash(component)"] --> B["call component.write_gds(no_empty_cells=True, with_metadata=False)"]
B --> C["receive gdspath string"]
C --> D["convert to Path(gdspath)"]
D --> E["read_bytes() from GDS file"]
E --> F["compute md5 hash of bytes"]
F --> G["unlink() temporary GDS file"]
G --> H["return hash string"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- You still convert
dirpathto aPathtwice in_get_sparameters_path; consider doing this once near the top of the function to avoid redundant work and keep the logic clearer. - In
get_component_hash, you might consider using a context-managed temporary file (e.g.,tempfile.NamedTemporaryFile) or an in-memory GDS export if available, to avoid relying on manual deletion and make the cleanup more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You still convert `dirpath` to a `Path` twice in `_get_sparameters_path`; consider doing this once near the top of the function to avoid redundant work and keep the logic clearer.
- In `get_component_hash`, you might consider using a context-managed temporary file (e.g., `tempfile.NamedTemporaryFile`) or an in-memory GDS export if available, to avoid relying on manual deletion and make the cleanup more robust.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
joamatab
approved these changes
Feb 28, 2026
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
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.
Currently, metadata generated by kfactory in the GDS file used to hash the simulated component can cause cache miss when changing gdsfactory or kfactory version or changing order of certain operations (which change neither component nor GDS shapes).
With this PR, metadata is not written to GDS for hashing, so caching should be more stable.
This is tested for
gmeep, but should work the same for other simulation tools.Summary by Sourcery
Stabilize s-parameter caching by ensuring component GDS exports used for hashing are written without extraneous metadata and consistently handled paths.
Enhancements: