-
Notifications
You must be signed in to change notification settings - Fork 6
add test for GDSFactory+ #192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideUpdates the GitHub Actions test workflow to inject a GFP API key into the environment and add a dedicated job that installs dependencies and runs the File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider scoping
GFP_API_KEYto just thetest_gdsfactoryplusjob (or even a single step) instead of defining it at the workflow level, to minimize exposure of the secret. - The
matrixdefinition intest_gdsfactoryplusonly has one Python version and one OS; if you don't plan to expand it, you can simplify by inliningpython-version: 3.12andruns-on: ubuntu-latestwithout a matrix. - If
test_gdsfactoryplusdepends on the other test jobs completing successfully, consider adding aneeds:clause so its execution order is explicit and future changes to the workflow don't accidentally change behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping `GFP_API_KEY` to just the `test_gdsfactoryplus` job (or even a single step) instead of defining it at the workflow level, to minimize exposure of the secret.
- The `matrix` definition in `test_gdsfactoryplus` only has one Python version and one OS; if you don't plan to expand it, you can simplify by inlining `python-version: 3.12` and `runs-on: ubuntu-latest` without a matrix.
- If `test_gdsfactoryplus` depends on the other test jobs completing successfully, consider adding a `needs:` clause so its execution order is explicit and future changes to the workflow don't accidentally change behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Updated Python version handling and improved workflow steps.
9135993 to
77f20c9
Compare
77f20c9 to
01ac5fd
Compare
| test_gdsfactoryplus: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.12"] | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7.1.3 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| - name: Test GDSFactory+ | ||
| run: uv run --group gdsfactoryplus gfp test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GDSFactory+ is running but failing. What is it exactly doing? It seems to fail at many sections where it assumes a layer or cross-section from the generic PDK would be available here. A SLAB90 layer in this PDK does not make sense for example.
.venv/lib/python3.12/site-packages/gdsfactory/pdk.py:713: in get_layer
return get_active_pdk().get_layer(layer)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
.venv/lib/python3.12/site-packages/gdsfactory/pdk.py:524: in get_layer
raise ValueError(f"{layer!r} not in PDK {self.name!r} {layer_members}")
E ValueError: 'SLAB90' not in PDK 'qpdk' OrderedDict({'M1_DRAW': <LayerMapQPDK.M1_DRAW: 1>, 'M1_ETCH': <LayerMapQPDK.M1_ETCH: 47>, 'M2_DRAW': <LayerMapQPDK.M2_DRAW: 3>, 'M2_ETCH': <LayerMapQPDK.M2_ETCH: 48>, 'AB_DRAW': <LayerMapQPDK.AB_DRAW: 49>, 'AB_VIA': <LayerMapQPDK.AB_VIA: 50>, 'JJ_AREA': <LayerMapQPDK.JJ_AREA: 13>, 'JJ_PATCH': <LayerMapQPDK.JJ_PATCH: 51>, 'IND': <LayerMapQPDK.IND: 52>, 'TSV': <LayerMapQPDK.TSV: 53>, 'DICE': <LayerMapQPDK.DICE: 38>, 'ALN_TOP': <LayerMapQPDK.ALN_TOP: 54>, 'ALN_BOT': <LayerMapQPDK.ALN_BOT: 55>, 'TEXT': <LayerMapQPDK.TEXT: 56>, 'LABEL_SETTINGS': <LayerMapQPDK.LABEL_SETTINGS: 30>, 'LABEL_INSTANCE': <LayerMapQPDK.LABEL_INSTANCE: 46>, 'SIM_AREA': <LayerMapQPDK.SIM_AREA: 57>, 'SIM_ONLY': <LayerMapQPDK.SIM_ONLY: 58>, 'WG': <LayerMapQPDK.WG: 59>})There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flaport any idea why this is happening?
Summary by Sourcery
Add CI coverage for the gdsfactoryplus (gfp) test command in the GitHub Actions workflow.
CI:
uv run gfp teston Python 3.12.