-
Notifications
You must be signed in to change notification settings - Fork 17
refactor!: Store function argument names in FuncInput
#1286
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
|
This PR contains breaking changes to the public Python API. Breaking changes summary |
|
| Branch | refactor/input-names |
| Testbed | Linux |
🚨 2 Alerts
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | hugr_nodes nodes x 1e3 | 📈 plot 🚷 threshold 🚨 alert (🔔) | 6.59 x 1e3(+14.98%)Baseline: 5.73 x 1e3 | 5.79 x 1e3 (113.84%) |
| tests/benchmarks/test_big_array.py::test_big_array_compile | hugr_bytes bytes x 1e3 | 📈 plot 🚷 threshold 🚨 alert (🔔) | 142.66 x 1e3(+5.00%)Baseline: 135.86 x 1e3 | 137.22 x 1e3 (103.96%) |
Click to view all benchmark results
| Benchmark | hugr_bytes | Benchmark Result bytes x 1e3 (Result Δ%) | Upper Boundary bytes x 1e3 (Limit %) | hugr_nodes | Benchmark Result nodes (Result Δ%) | Upper Boundary nodes (Limit %) |
|---|---|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 142.66 x 1e3(+5.00%)Baseline: 135.86 x 1e3 | 137.22 x 1e3 (103.96%) | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 6,590.00(+14.98%)Baseline: 5,731.50 | 5,788.81 (113.84%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 21.57 x 1e3(-0.97%)Baseline: 21.78 x 1e3 | 22.00 x 1e3 (98.05%) | 📈 view plot 🚷 view threshold | 606.00(-6.70%)Baseline: 649.50 | 656.00 (92.38%) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1286 +/- ##
==========================================
- Coverage 93.51% 93.50% -0.01%
==========================================
Files 123 123
Lines 11513 11520 +7
==========================================
+ Hits 10766 10772 +6
- Misses 747 748 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6e6e2c2 to
4fb6328
Compare
|
|
||
| def signature_to_str(name: str, sig: FunctionType) -> str: | ||
| """Displays a function signature in Python syntax including the function name.""" | ||
| assert sig.input_names is not None |
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.
this line shouldn't have to change
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.
because input_names is a property of type Sequence[str] | None that returns None if any elements are unnamed...yes, true, but not necessarily that obvious . See comment on def input_names but maybe go with Craig ;)
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.
Thanks @mark-koch, looks like a good change :). A couple of queries but generally happy to approve
|
|
||
| def signature_to_str(name: str, sig: FunctionType) -> str: | ||
| """Displays a function signature in Python syntax including the function name.""" | ||
| assert sig.input_names is not None |
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.
because input_names is a property of type Sequence[str] | None that returns None if any elements are unnamed...yes, true, but not necessarily that obvious . See comment on def input_names but maybe go with Craig ;)
|
|
||
| @cached_property | ||
| def input_names(self) -> Sequence[str] | None: | ||
| """Names of all inputs or `None` if there are unnamed inputs.""" |
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.
Is it worth adding a def __post_init__ to check that either all have names or none have names? I'm not sure I see any cases where only some might be named....perhaps positional-only arguments??
|
|
||
| #: Name of this input, or `None` if it is an unnamed argument (e.g. inside a | ||
| #: higher-order `Callable` type) | ||
| name: str | None = field(default=None, compare=False) |
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.
Why compare=False? So function types are equal if one has names and the other does not? Not strictly correct - the former can be called with kwargs syntax whereas the latter does not - I'm not sure how much of that stays in guppy but if we do support that, it might be worth going to a more-full-blown FunctionType "generalizes" check allowing assignment of named to nameless but not the other way around?
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.
Guppy currently doesn't support kwargs, so function names are irrelevant when it comes to calling. The same is also true when e.g. trying to unify to function types - we just ignore the names
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.
Ah! Very good. In that case quite right. Maybe worth a # not exposed to caller here then
| inp.flags, | ||
| is_func_input=True, | ||
| ) | ||
| for i, inp in enumerate(func_ty.inputs) |
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.
| for i, inp in enumerate(func_ty.inputs) | |
| for arg, inp in zip(func_def.args.args, func_ty.inputs) |
possibly with strict=True
|
Apologies, totally forgot about this PR 😅 |
It't a lot nicer to have the name of function arguments in the same object as the type an flags instead of the separate list we have at the moment - not adding the name was more of an oversight when we introduced
FuncInput😅BREAKING CHANGE:
FunctionTypeconstructor no longer accepts theinput_namesargument. Instead, input names should be provided as an optional argument toFuncInput