Repository navigation
gdstk backend: close API gaps so the tutorials run - #104
carloscl03 wants to merge 8 commits into
Conversation
|
Great catch. Thanks! |
|
Can you please fix the merge conflicts? |
6da8d59 to
2677d02
Compare
|
Rebased onto current Two commits were dropped rather than carried over: both touched On the snap-to-grid commit in particular, one measurement that may be worth knowing. The DRC/LVS workflows run the gdsfactory backend, so gdstk is not covered by CI. Building the gf180 cells on gdstk with #102 applied gets them to build, but DRC then reports off-grid geometry: That is the class this commit removes: |
|
One more thing on the relationship with #102, since this branch is the approved one and will likely land first. The two conflict in
Verified with that resolution on top of
No behaviour change against the tree those were first measured on. Detail left here as well as on #102 so it is visible from whichever side is merged second — I can rebase #102 once this one is in. |
|
Pushed one more commit here:
That was the last gap stopping the tutorial suite from running on gdstk. Measured on this fork's CI, with this PR plus #102 and #113 applied:
This commit touches only the backend, so it does not change what this PR's own CI exercises — the notebooks job here runs gdsfactory either way. |
Two notebooks sit on the 180s per-cell limit and cross it depending on
which runner the job lands on, so the check passes or fails on identical
content. `lvs-pin-filter` has four green runs and two red ones.
GLayout_Cells 290s pass (#112) / 372s error (#113)
glayout_opamp 136s pass (#112) / 198s error (#113)
The native backend builds these cells several times faster, which moves
them off the limit instead of raising it. Measured on this fork's CI --
same container, same runners -- with #102, #104 and #113 applied on both
sides, the only difference being this line:
gdsfactory gdstk
GLayout_Cells 187.8s ERROR 14.2s pass
glayout_opamp 126.4s pass 19.2s pass
5T_OTA_part2 90.8s pass 16.2s pass
test_bjt_custom_pattern 68.2s pass 11.4s pass
whole suite 660s 13/14 158s 14/14
Note the baseline already carries all three PRs and still fails: they are
what lets the notebooks run on gdstk at all, not what fixes the timeouts.
Depends on #102, #104 and #113. Without #113 `GLayout_Cells` raises in
`mimcap.py`; without #102 it raises in `Component.add`; without #104's
`add_ref(columns=)` the BJT notebook raises. This check stays red until
all three land.
5ebee1d to
e6144e0
Compare
|
Hi Prof. @msaligane @AL-255 I think this is a good addition. Maybe it is worth checking why it was switched to gdsfactory in case gdstk was already available in the past or if not the case, why was not used in first place. Thanks, |
|
@carloscl03 @LuighiV My understanding is gdstk might be faster to run but has some rounding issues. But I am not fully sure. |
Running the tutorial notebooks under GLAYOUT_BACKEND=gdstk fails almost immediately: 4 of 14 pass. Two reasons, one on each side. The notebooks import Component, rectangle, boolean and cell straight from gdsfactory instead of glayout.backend, so they bypass the backend selection entirely. Under the default backend both names resolve to the same class and nobody notices; under gdstk, gdsfactory's add_ref() gets a gdstk Component and rejects it. Symbols glayout.backend does not re-export (text_freetype, array) are left on gdsfactory. The gdstk ComponentReference/Component are also missing pieces of the gdsfactory surface that glayout's own cells and the tutorials use: movex/movey took a bare delta; move() already accepted destination= ref.name read-only, but cells label placements (ref.name = "pfet_2") ref.x / ref.y had xmin/xmax/ymin/ymax but not the centre accessors write_gds required a filename and ignored gdsdir= Component.show absent ref.name stores the label on the reference rather than renaming the target cell, which would rename every other placement of it too. Tutorial notebooks under gdstk: 4/14 -> 10/14. The three BJT tutorials still fail (Component indexing, add_ref(columns=)) and are untouched here. Notebooks under the default gdsfactory backend are unaffected: the imports resolve to the same objects.
Every vertex of a gdstk-generated layout lands off-grid on gf180. The DRC
reports it on all of them -- 1301 violations on the LIF cell used to check
this, split across contact_OFFGRID x348, via1_OFFGRID x276,
metal1_OFFGRID x252, metal2_OFFGRID x132 and comp_OFFGRID x60.
snap_to_grid() took a bare `nm: int = 1` default. gdsfactory's version reads
the pitch from the active PDK instead:
nm = int(get_grid_size() * 1000 * grid_factor)
which is 5 nm on gf180. Rounding a 5 nm process to 1 nm produces values like
10.246 where the process wants 10.245, and every one of them is a violation.
Two pieces were missing. Pdk.activate() was a no-op, so nothing recorded
which PDK was active; and grid_size kept the class default of 0.001 because
gdsfactory used to fill it in from its own PDK database on activate. The
real pitch is already in gds_write_settings.precision (5e-9 m on both gf180
and sky130), so activate() now derives grid_size from it and registers the
PDK for snap_to_grid to read. `nm=` still overrides when a caller wants a
specific pitch.
After: 0 of 1228 vertices off-grid, same as the gdsfactory backend, and the
1301 OFFGRID violations are gone. snap_to_2xgrid(10.2463) returns 10.25 on
both backends now. DRC on diff_pair, current_mirror_nfet and
transmission_gate under gdsfactory is unchanged.
move(destination=) translated relative to the bbox centre instead of (0,0), which is gdsfactory's default. c_route places its extension rectangles with move(destination=...) followed by relative movex, so the route shifted by half a rectangle: the LIF neuron came out 32.4 um wide instead of 29.9, with 54 extra M2.2a violations. get_ports_list returned dict order; gdsfactory sorts clockwise (west, north, east, south). Cells look ports up by substring, so the order changes which port gets routed. With both, neurona.ipynb gives 1168.6 um2 and 31 violations on either backend, identical to gdsfactory.
Two gaps that show up running the team's primitives notebook. add_ports iterated its argument assuming a sequence of Port, but gdsfactory also accepts a name->port mapping and glayout passes ref.ports straight in. Iterating that yields the names, so resistor() died with "'str' object has no attribute 'name'". And __getitem__ was missing. bjt indexes the component to read a port's width, and without it the error is "'Component' object is not subscriptable", which says nothing about the port it was after. Added on Component and ComponentReference, with the KeyError listing the ports that do exist. With these, resistor, pnp and npn build on the gdstk backend.
`test_bjt_gdsfactory` lays its contact rings out with `add_ref(reference, columns=, rows=, spacing=)`, which gdsfactory accepts and this backend did not, so the notebook died on a TypeError partway through. gdstk.Reference has the same notion natively, so the array stays one reference rather than becoming rows*columns of them. Verified in CI on the fork: with this, the tutorial suite runs 14/14 on gdstk. Without it, 13/14 -- that notebook the only failure.
e6144e0 to
857b6fb
Compare
The rounding this PR fixes had no test, so nothing stops a 1 nm default from coming back. Four checks, in the order the failure actually happens: - activate() derives grid_size from precision (5e-9 m -> 0.005 um), so the pitch is never written down in a second place that can drift. - snap_to_grid puts 10.2463 at 10.245, not at 10.246. That value is legal on a 1 nm grid and off-grid on gf180's 5 nm one, which is the whole bug. - an explicit nm= still overrides, so callers that need another pitch keep working. - every vertex of a built nmos(fingers=2) lands on the 5 nm grid. Snapping scalars right is not the same as a device coming out clean: the generators compose dozens of snapped values, and this is the check that matches what the DRC reports. Verified to fail without the fix, by making activate() skip the grid correction: 3 of the 4 fail, the built device showing 3156 off-grid vertices. The override check passes either way by design -- it guards behaviour this PR keeps.
The tutorial job hangs intermittently at the 180s cell timeout, on whichever cell happens to be waiting. Instrumented on the runner itself (DEBUG logs with timestamps, 8 runs): the kernel finishes the cell, sends idle, and the reply goes missing -- sometimes surfacing ~179s later, sometimes never, in which case the next execute_request is also lost and the run dies. The trigger is the display helpers: display_component draws the whole opamp into a ~2.7MB SVG per message. A/B on the same machine, same notebook: 3/3 runs with displays stalled or died, 3/3 without them ran clean in 12.5s. Three layers, each measured: - --strip-displays: the harness executes a copy with display lines dropped (syntax-guarded per cell). CI executes for regressions, not for pictures. - jupyter-client pinned to 8.9.1: 8.10.0 (released 2026-08-28 12:17 UTC) quadrupled the loss rate, 4/6 against 1/6, measured A/B. This is also why the same commit passed on the 25th and hung on the 28th: the install is unpinned, so the environment drifts with PyPI. - --retries 1: only for the "error" class (crash/timeout), never for a cell failure, so a genuinely broken notebook still fails every attempt. klayout was ruled out (10/10 bare runs at 1.5s on the runner), as were the runners themselves (identical container and venv timings on green and red runs) and the PR content (byte-identical opamp SVG across runs).
|
@msaligane Both points are addressed, and the notebooks are green. Two commits pushed. Merge conflictsResolved; the branch is mergeable again. The conflict was with #102 in The rounding issueYour instinct was right, and it is what commit It had no test, so nothing stopped a 1 nm default from coming back.
Verified to fail without the fix by making The notebooks14/14, and it holds: five consecutive suites on my fork, the last three at 143–150 s. That job was hanging on whichever cell happened to be running. Instrumented on the runner with timestamped DEBUG logs: the kernel finishes the cell, sends idle, and the reply goes missing — sometimes surfacing ~179 s later, sometimes never, in which case the next request is lost too and the run dies at the 180 s timeout. Two causes, each measured A/B on the same machine:
Point 2 also answers why the same commit passed on the 25th and hung on the 28th. The venv cache key hashes
Ruled out along the way: klayout (10/10 bare runs at ~1.5 s on the runner), gdstk, the runner class, and this PR's own content — the opamp SVG is byte-identical between passing and failing runs. Happy to split the CI work into its own PR if you would rather keep this one to the backend; it is two commits and rebases cleanly either way. |
PR ReaLLMASIC#125 has the better diagnosis. The tutorials wrap generation in `with widgets.Output():`, and under headless nbconvert that capture makes nbclient wait out the full per-cell timeout even though the kernel is already idle -- 10 min per widget cell against 11 s without it. That is a far larger effect than anything measured here. My own A/B was confounded: removing `display_component` removed the multi-MB SVG *and* the widget-capture cell at the same time, so it could not tell them apart. And stripping the displays changed what CI exercises -- the drawing path stopped being covered at all -- where ReaLLMASIC#125 keeps the displays and disables only the capture. Theirs is the better fix, and `_strip_displays` inserted at the same line of run_tutorial_notebooks.py that ReaLLMASIC#125 inserts into, so this also removes the conflict. Kept, because ReaLLMASIC#125 does not have them and they are independent of which diagnosis is right: - jupyter-client==8.9.1, plus a step that enforces it on cache hits. The cache key hashes setup.py and src/, not the workflow, so a pin written only in the install step is inert when the venv is restored. That is how 8.10.0 got in: a src change busted the key on 2026-08-28 and the cold rebuild took the version published that morning. - --retries 1 for the crash/timeout class only. A failing cell is never retried, so a genuinely broken notebook still fails every try.
Running the tutorial notebooks under
GLAYOUT_BACKEND=gdstkfails almostimmediately — 4 of 14 pass. Two causes, one on each side.
The notebooks bypass the backend selection
Under the default backend this resolves to the same class as
glayout.backend.Component, so nobody notices. Under gdstk, gdsfactory'sadd_ref()receives a gdstkComponentand rejects it:Redirected
Component,rectangle,rectangular_ring,booleanandcelltoglayout.backend. Symbols it does not re-export(
text_freetype,array) stay on gdsfactory. Imports only — no cellcontent touched.
The gdstk backend is missing parts of the surface
Each of these is used by glayout's own cells or by the tutorials:
movex/moveydestination=too, likemove()already hadref.nameref.name = "pfet_2")ref.x/ref.yxmin/xmaxwrite_gdsfilenamerequired, nogdsdirComponent.showref.namestores the label on the reference rather than renaming thetarget cell — renaming the cell would rename every other placement of it.
Result
The three BJT tutorials still fail on
Componentindexing andadd_ref(columns=); those need more than a signature and are left alone.Notebooks under the default gdsfactory backend are unaffected — the
redirected imports resolve to the same objects.
Worth noting for #100:
glayout_opamp.ipynbandGLayout_Cells.ipynb,which currently time out at 180 s under gdsfactory, complete in 89 s and
53 s under gdstk.