Harden all_cells parameter filtering, return-type handling, and packing defaults

[Copilot]

qpdk/samples/all_cells.py had three correctness/readability gaps: required-parameter detection relied on parameter name matching, non-Component return handling assumed add_ref_off_grid compatibility, and the unconstrained max_size default was undocumented. This PR tightens parameter/return-type checks and documents default packing behavior explicitly.

• Parameter introspection correctness

  • Replaced name-based exclusion of **kwargs with kind-based filtering (inspect.Parameter.VAR_KEYWORD) when computing required params.
  • This avoids false positives from parameter naming and aligns with Python’s signature model.

• Safer handling of non-Component cell returns

  • Added an explicit type gate before wrapping:
    • gf.Component → used directly
    • gf.ComponentReference → wrapped via add_ref_off_grid
    • any other type → skipped with warning including the actual type name
  • Prevents implicit interface assumptions and avoids AttributeError-style failures on unsupported return objects.

• Documented unconstrained packing default

  • Updated docstring for **kwargs to explain default max_size=(None, None).
  • Added inline comment at setdefault call to make the intent clear at the point of behavior.

• Focused regression coverage

  • Added tests/test_all_cells_sample.py to cover:
    • **kwargs not treated as required parameters
    • unsupported return types are warned/skipped
    • max_size defaults to unconstrained and respects explicit override

required_params = [
    p
    for p in sig.parameters.values()
    if p.default == inspect.Parameter.empty
    and p.kind != inspect.Parameter.VAR_KEYWORD
]

if not isinstance(cell, gf.Component):
    if not isinstance(cell, gf.ComponentReference):
        warnings.warn(
            f"Skipping cell '{name}': unsupported return type {type(cell).__name__}",
            UserWarning,
            stacklevel=2,
        )
        continue
    c_wrap = gf.Component(name=f"{name}_wrap")
    c_wrap.add_ref_off_grid(cell)

Original prompt

Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The check `p.name != 'kwargs'` may not correctly filter variadic keyword parameters. Use `p.kind != inspect.Parameter.VAR_KEYWORD` instead to properly exclude **kwargs parameters from required parameter detection.","fixFiles":[{"filePath":"qpdk/samples/all_cells.py","diff":"diff --git a/qpdk/samples/all_cells.py b/qpdk/samples/all_cells.py\n--- a/qpdk/samples/all_cells.py\n+++ b/qpdk/samples/all_cells.py\n@@ -61,7 +61,7 @@\n         required_params = [\n             p\n             for p in sig.parameters.values()\n-            if p.default == inspect.Parameter.empty and p.name != \"kwargs\"\n+            if p.default == inspect.Parameter.empty and p.kind != inspect.Parameter.VAR_KEYWORD\n         ]\n \n         if required_params:\n"}]},{"message":"The method `add_ref_off_grid` is used without validation of whether `cell` supports this operation. Consider adding a check or handling potential AttributeError if the cell object doesn't support the expected interface.","fixFiles":[{"filePath":"qpdk/samples/all_cells.py","diff":"diff --git a/qpdk/samples/all_cells.py b/qpdk/samples/all_cells.py\n--- a/qpdk/samples/all_cells.py\n+++ b/qpdk/samples/all_cells.py\n@@ -78,6 +78,13 @@\n                 continue\n \n             if not isinstance(cell, gf.Component):\n+                if not isinstance(cell, gf.ComponentReference):\n+                    warnings.warn(\n+                        f\"Skipping cell '{name}': unsupported return type {type(cell).__name__}\",\n+                        UserWarning,\n+                        stacklevel=2,\n+                    )\n+                    continue\n                 c_wrap = gf.Component(name=f\"{name}_wrap\")\n                 c_wrap.add_ref_off_grid(cell)\n                 cell = c_wrap\n"}]},{"message":"The purpose and effect of setting `max_size` to `(None, None)` is not documented. This default configuration should be explained in the docstring or with an inline comment, as it affects the packing behavior in a non-obvious way.","fixFiles":[{"filePath":"qpdk/samples/all_cells.py","diff":"diff --git a/qpdk/samples/all_cells.py b/qpdk/samples/all_cells.py\n--- a/qpdk/samples/all_cells.py\n+++ b/qpdk/samples/all_cells.py\n@@ -28,7 +28,9 @@\n \n     Args:\n         spacing: Spacing between cells in micrometers (default: 200.0).\n-        **kwargs: Additional arguments passed to gf.pack.\n+        **kwargs: Additional arguments passed to gf.pack. If ``max_size`` is not\n+            provided, it defaults to ``(None, None)`` so packing is unconstrained\n+            in both dimensions unless the caller explicitly sets limits.\n \n     Returns:\n         Component containing all successfully instantiated cells.\n@@ -94,6 +96,8 @@\n     if not cells:\n         return Component(\"empty_all_cells\")\n \n+    # Use unconstrained packing by default to avoid implicit size limits unless\n+    # callers explicitly provide `max_size`.\n     kwargs.setdefault(\"max_size\", (None, None))\n     bins = gf.pack(cells, spacing=spacing, **kwargs)\n \n"}]}]

Summary by Sourcery

Harden all_cells sample parameter detection, return-type handling, and default packing behavior.

Bug Fixes:

• Correct required-parameter detection to ignore variadic keyword arguments based on their kind rather than name.
• Prevent failures when cell factories return unsupported types by warning and skipping them instead of assuming ComponentReference compatibility.

Enhancements:

• Clarify and enforce that all_cells defaults gf.pack max_size to unconstrained (None, None) while still honoring explicit overrides.

Tests:

• Add regression tests covering **kwargs exclusion from required parameters, warnings for unsupported return types, and default vs explicit max_size behavior in packing.

[sourcery-ai]

Reviewer's Guide

Tightens how qpdk.samples.all_cells introspects cell callables, handles non-Component return types, and configures default packing behavior, with focused regression tests to prevent regressions.

Flow diagram for updated all_cells cell handling and packing defaults

flowchart TD
    A[Iterate over cell_funcs] --> B[Call cell_func]
    B --> C{_is_qpdk_cell returns True?}
    C -->|No| A
    C -->|Yes| D{cell is gf.Component?}
    D -->|Yes| G[Append cell to cells]
    D -->|No| E{cell is gf.ComponentReference?}
    E -->|No| F[warnings.warn and skip]
    F --> A
    E -->|Yes| H[Create gf.Component name=name_wrap]
    H --> I[c_wrap.add_ref_off_grid cell]
    I --> J[Set cell to c_wrap]
    J --> G
    G --> K{More cell_funcs?}
    K -->|Yes| A
    K -->|No| L{cells list empty?}
    L -->|Yes| M[Return Component empty_all_cells]
    L -->|No| N[kwargs.setdefault max_size to None None]
    N --> O[gf.pack cells spacing=spacing with kwargs]
    O --> P[Return packed Component]

Loading

File-Level Changes

Change	Details	Files
Fix required-parameter detection to correctly ignore **kwargs when classifying qpdk cell functions.	Compute required parameters using inspect.Parameter.kind != VAR_KEYWORD instead of filtering by name 'kwargs'. Leave behavior for other parameter kinds and default-less parameters unchanged so only true required args trigger warnings.	qpdk/samples/all_cells.py
Harden handling of non-Component return values from cell functions to avoid unsafe assumptions about add_ref_off_grid support.	Introduce explicit type gate: accept gf.Component directly, wrap gf.ComponentReference in a new wrapper component, and skip any other types. Emit a UserWarning when skipping unsupported return types, including the concrete type name in the message. Maintain existing wrapping behavior for ComponentReference by creating a wrapper Component and calling add_ref_off_grid before continuing.	qpdk/samples/all_cells.py
Document and enforce an unconstrained default packing size, while preserving caller overrides.	Expand all_cells docstring to explain that max_size defaults to (None, None), meaning unconstrained packing unless explicitly set. Add an inline comment near kwargs.setdefault to document the intent at the call site. Use kwargs.setdefault('max_size', (None, None)) so explicit max_size arguments from callers take precedence over the default.	qpdk/samples/all_cells.py
Add targeted tests to lock in the new introspection, type-handling, and packing default behaviors.	Create tests ensuring **kwargs-only parameters are not treated as required while genuinely required params still emit warnings. Add tests verifying unsupported return types are skipped with a warning that contains the concrete type name. Add tests confirming all_cells sets max_size=(None, None) when not provided and respects explicit max_size overrides, capturing gf.pack kwargs via a monkeypatched implementation.	tests/test_all_cells_sample.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_all_cells_sample.py" line_range="18-27" />
<code_context>
+    return func
+
+
+def test_all_cells_does_not_treat_var_keyword_as_required(recwarn, monkeypatch) -> None:
+    """Ensure variadic keyword parameters are not treated as required arguments."""
+
+    @_mark_as_qpdk_cell
+    def accepts_var_keyword(**options):
+        _ = options
+        return gf.Component()
+
+    @_mark_as_qpdk_cell
+    def requires_argument(length):
+        _ = length
+        return gf.Component()
+
+    monkeypatch.setattr(
+        qpdk,
+        "PDK",
+        SimpleNamespace(cells={"accepts_var_keyword": accepts_var_keyword, "requires_argument": requires_argument}),
+    )
+
+    _ = all_cells(spacing=201)
+
+    warning_messages = [str(w.message) for w in recwarn]
+    assert any("requires arguments ['length']" in msg for msg in warning_messages)
+    assert not any("requires arguments ['options']" in msg for msg in warning_messages)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting on the resulting component to fully validate the `**kwargs` handling path.

You already verify that `**options` isn’t treated as a required arg via warnings. To strengthen this test, also assert something about the `all_cells` result—for instance, that it returns a non-empty component set or a specific number of placed cells—so we confirm the var-keyword-only path is functionally correct, not just warning-free.

Suggested implementation:

```python
    result = all_cells(spacing=201)

    assert isinstance(result, gf.Component)
    assert len(result.references) == 2

    warning_messages = [str(w.message) for w in recwarn]
    assert any("requires arguments ['length']" in msg for msg in warning_messages)
    assert not any("requires arguments ['options']" in msg for msg in warning_messages)

```

If `all_cells` returns a different structure than a single `gf.Component` (e.g., a container object or a list of components), update the assertions accordingly:
1. Change `isinstance(result, gf.Component)` to match the actual return type.
2. Replace `len(result.references) == 2` with an assertion that checks the expected number of placed cells/components in the returned structure, still validating that both `accepts_var_keyword` and `requires_argument` were instantiated.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Consider asserting on the resulting component to fully validate the **kwargs handling path.

You already verify that **options isn’t treated as a required arg via warnings. To strengthen this test, also assert something about the all_cells result—for instance, that it returns a non-empty component set or a specific number of placed cells—so we confirm the var-keyword-only path is functionally correct, not just warning-free.

Suggested implementation:

    result = all_cells(spacing=201)

    assert isinstance(result, gf.Component)
    assert len(result.references) == 2

    warning_messages = [str(w.message) for w in recwarn]
    assert any("requires arguments ['length']" in msg for msg in warning_messages)
    assert not any("requires arguments ['options']" in msg for msg in warning_messages)

If all_cells returns a different structure than a single gf.Component (e.g., a container object or a list of components), update the assertions accordingly:

1. Change isinstance(result, gf.Component) to match the actual return type.
2. Replace len(result.references) == 2 with an assertion that checks the expected number of placed cells/components in the returned structure, still validating that both accepts_var_keyword and requires_argument were instantiated.

[github-code-quality]

Code Coverage Overview

Languages: Python

Python / code-coverage/pytest

The overall coverage in the copilot/fix-paramete... branch remains at 90%, unchanged from the main branch.

Show a code coverage summary of the most impacted files.

File	main 90946c4	copilot/fix-paramete... 49cd49a	+/-
samples/all_cells.py	82%	81%	-1%

---

_{Code Coverage is in Public Preview. Learn more and provide us with your feedback.}