feat!: Allow specifying link names for lowered functions and declarations#1494
feat!: Allow specifying link names for lowered functions and declarations#1494maximilianruesch merged 26 commits intomainfrom
Conversation
|
This PR contains breaking changes to the public Python API. Breaking changes summary |
|
| Branch | mr/feat/pass-hugr-names |
| Testbed | Linux |
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 | 141.79 x 1e3(-0.01%)Baseline: 141.81 x 1e3 | 143.23 x 1e3 (99.00%) | 📈 view plot 🚷 view threshold | 6,620.00(0.00%)Baseline: 6,620.00 | 6,686.20 (99.01%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 17.57 x 1e3(-0.09%)Baseline: 17.59 x 1e3 | 17.76 x 1e3 (98.93%) | 📈 view plot 🚷 view threshold | 581.00(0.00%)Baseline: 581.00 | 586.81 (99.01%) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1494 +/- ##
==========================================
+ Coverage 93.39% 93.42% +0.03%
==========================================
Files 128 128
Lines 11970 12022 +52
==========================================
+ Hits 11179 11232 +53
+ Misses 791 790 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
acl-cqc
left a comment
There was a problem hiding this comment.
This is cracking stuff, thanks @maximilianruesch. Comments are all small/tiny, not suggesting any major changes :)....except possibly:
Should we rename (qualified_)hugr_name to link_name? I.e. if these declarations will also be used for binary linking?
The counterargument is that guppy puts these names only into hugr (it doesn't generate anything else) so whatever happens to hugr-names downstream from there isn't in guppy's domain....we might wanna check with @doug-q here
| object.__setattr__(self, "_user_set_hugr_name", hugr_name) | ||
|
|
||
| @cached_property | ||
| def qualified_hugr_name(self) -> str: |
There was a problem hiding this comment.
Nice, I like the use of cached_property too
There was a problem hiding this comment.
In hugr the field we are filling in is just name so this is ok (hugr_qualified_name would not be ok!!). I'd consider calling this just hugr_name as that might be more obvious (and avoid question of, is there a method returning the unqualified name), but this looks ok.
There was a problem hiding this comment.
Moreover, the same value is called hugr_name in the other classes
There was a problem hiding this comment.
There is a problem with clashing names when we have the hugr_name init variable. We could consider renaming that to sth like hugr_name_override, and then rename this property to hugr_name. I would actually like this. What do you think?
There was a problem hiding this comment.
I kinda like that, but it is a lot of typing for users, and I think "setting the hugr_name or leaving the default" (rather than "setting an override") is a reasonable mental model for them (i.e. they don't need to be concerned with quite how complex it is to calculate the default....)
I tried:
>>> @dataclass
... class Foo:
... xx: InitVar[str | None] = None
... def __post_init__(self, xx: str | None):
... print("Post init called with xx=",xx)
... @property
... def xx(self):
... return "world"
...
>>> Foo().xx
Post init called with xx= <property object at 0x105e4e020>
'world'
>>> Foo(xx="hello").xx
Post init called with xx= hello
'world'
(same with xx: InitVar[str | None] = field(default = None))
Not sure what that property object is but I don't see a problem with having two xxs...have you tried?
There was a problem hiding this comment.
I have not actually tried, but some type checker did complain. I will give it a shot and try to identify the root cause 👍
There was a problem hiding this comment.
We can do this, but we will have to disable ruffs F811 rule for "redefined while unused" checks, since it does not seem to pick up on initvars yet.
Mypy also complains with "already defined", and later with "has no attribute" when trying to access it. I do not think this pattern is widely compatible with the tools that we use.
| func_ast, | ||
| ty, | ||
| docstring, | ||
| self.qualified_hugr_name, |
There was a problem hiding this comment.
It's a lot of parameters, we want might to start using more kwargs at some point
There was a problem hiding this comment.
At some point is a good point. I did have some issues with kwargs and type checkers, so I just avoided them for the moment.
| .. code-block:: python | ||
| from guppylang import guppy | ||
|
|
||
| @guppy.struct |
There was a problem hiding this comment.
maybe give example of passing parameter? (@guppy.struct(....) \\ class MyStruct2)
|
|
||
|
|
||
| def test_func_hugr_name_annotated(): | ||
| """Asserts that annotated HUGR func names are correctly passed to the HUGR nodes.""" |
There was a problem hiding this comment.
| """Asserts that annotated HUGR func names are correctly passed to the HUGR nodes.""" | |
| """Asserts that annotated function `hugr_name`s are correctly passed to the HUGR nodes.""" |
| def crazy_dec() -> None: ... | ||
|
|
||
| assert _func_names(crazy_def.compile()) == { | ||
| "tests.integration.test_hugr_name.test_func_hugr_name_inferred.<locals>.crazy_def" |
There was a problem hiding this comment.
It would be lovely to remove some of the tests.integration.test_hugr_name.test_func_hugr_name_inferred.<locals> stuff from the test even if not the hugr_name.
Is there anything we can do like test_func_hugr_name_inferred.__qual_name__ + ".<locals>.crazy_def"? Or maybe make a toplevel file_name = .... and then use file_name + "test_func_hugr_name_inferred.<locals>.crazy_def"?
Or def some_local(): pass then sthg like prefix = some_local.__full_name__.remove_suffix("some_local") (suspect that no such full-name exists but mentioning just in case it does)
E.g. to make the tests less fragile if we rename the file
There was a problem hiding this comment.
For removing fragility it would be sufficient to add some top level MODULE string that dynamically determines the module, and prepending that to every expected name.
For shortening the expected strings, we can use the request fixture from pytest to react to the test name.
I will cook up a custom fixture for this file 👍
There was a problem hiding this comment.
I think the MODULE would probably have 90% of the benefit and be significantly simpler/more obvious. There is probably something to be said for making the <locals> explicit so people are less surprised when they see it in practice...
There was a problem hiding this comment.
Then I would propose a middle ground test_qualifier fixture (which can be defined just as a simple function at the top of the file) that covers everything up to locals, and can be used to test a name as:
<my_expr> == f"{test_qualifier}.<locals>.main"There was a problem hiding this comment.
sounds fine although IIUC you'll be defining that once per function....at least MODULE is only once per file
There was a problem hiding this comment.
The trick is (using the fixture) I do not. See updated code, I can define
@pytest.fixture
def qualifier(request) -> str:
"""Provides the common qualifier for functions defined in the current test."""
def tmp() -> None:
pass
return f"{tmp.__module__}.{request.node.originalname}"
which processes the test name I receive from pytest.
| from guppylang import guppy | ||
|
|
||
|
|
||
| def _func_names(package: Package) -> set[str]: |
There was a problem hiding this comment.
Consider defining func_names_excluding_main that checks-existence+removes exactly one name ending in main
acl-cqc
left a comment
There was a problem hiding this comment.
This looks pretty good for me, I'll hold back approval only until I've heard from Doug about hugr_name vs link_name vs other
| func_def, | ||
| func_ty, | ||
| None, | ||
| hugr_name=func_def.name, |
There was a problem hiding this comment.
perhaps worth a comment that since nested functions are always private this doesn't really matter
| if self._user_set_hugr_name is not None: | ||
| hugr_name = self._user_set_hugr_name | ||
| else: | ||
| parent_ty_id = DEF_STORE.impl_parents.get(self.id) |
There was a problem hiding this comment.
style: I think if (parent_ty_id := DEF_STORE.impl_parents.get(self.id)) is not None: is fine, and allows reducing the indent via elif. Whether elif parent_ty_id := DEF_STORE.impl_parents.get(self.id) (dropping the explicit is not None) is good is a different question...
|
|
||
| @dataclass(frozen=True) | ||
| class CheckedFunctionDecl(RawFunctionDecl, CompilableDef, CallableDef): | ||
| class CheckedFunctionDecl(CompilableDef, CallableDef): |
There was a problem hiding this comment.
Dropping the RawFunctionDecl as a superclass seems quite a significant change, what happens to fields python_func, description, unitary_flags? Are they multiply-inherited?
There was a problem hiding this comment.
I dropped python_func on purpose, since similar to ParsedFunctionDef the original function object is not required anymore, and instead its parsed AST version is passed via defined_at.
unitary_flags is, again similar to ParsedFunctionDef, packaged into the function type passed via ty (and that is inherited from CallableDef).
description was not populated by the parsing process, and never used, so I dropped it without replacement.
There was a problem hiding this comment.
A significant (driveby?) improvement/cleanup, SGTM
| return self.field2 + self.field2 | ||
|
|
||
| # Add optional parameters | ||
| @guppy.struct(...) |
There was a problem hiding this comment.
| @guppy.struct(...) | |
| @guppy.struct(hugr_name="my_struct") |
acl-cqc
left a comment
There was a problem hiding this comment.
Looks great, thanks @maximilianruesch, just a couple of suggestions. I love that your test function names are even betterly qualified than mine ;-)
|
|
||
| @dataclass(frozen=True) | ||
| class CheckedFunctionDecl(RawFunctionDecl, CompilableDef, CallableDef): | ||
| class CheckedFunctionDecl(CompilableDef, CallableDef): |
There was a problem hiding this comment.
A significant (driveby?) improvement/cleanup, SGTM
| func_ast, globals, self.id, unitary_flags=self.unitary_flags | ||
| ) | ||
|
|
||
| link_name = f"{self.python_func.__module__}.{self.python_func.__qualname__}" |
There was a problem hiding this comment.
if you made a superclass, you could give it a method calculate_link_name using early return to avoid this set-default-and-override pattern
There was a problem hiding this comment.
With the fetching of the struct (which should be very specific and explicit for functions) it is imo rather awkward and not straight forward to raise into a superclass.
There was a problem hiding this comment.
Ah....that does reduce the value of raising the superclass. Hmmm, let me have another look
Allows annotating function names for definitions and declarations lowered to HUGR. This also changes the default inferral of names to be fully qualified, instead of only partially qualified as in #1452.
See tests for more info on the details of how qualification works.
Closes #1483
BREAKING CHANGE: ParsedFunctionDef and CheckedFunctionDecl have obtained a new required parameter that is set during parsing.
BREAKING CHANGE: The inheritance chain for function declarations (RawFunctionDecl, etc ...) was changed, behaving more similar to the inheritance chain for function definitions.
BEGIN_COMMIT_OVERRIDE
feat: Allow specifying link names for lowered functions and declarations (#1494)
END_COMMIT_OVERRIDE