Add support for Annotated types in plugins #8665

jptrindade · 2025-01-20T17:20:02Z

Description

Introduce Annotated types in plugins

closes #8573

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

Attached issue to pull request
Changelog entry
Type annotations are present
Code is clear and sufficiently documented
No (preventable) type errors (check using make mypy or make mypy-diff)
Sufficient test cases (reproduces the bug/tests the requested feature)
Correct, in line with design
End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

jptrindade · 2025-01-21T09:10:57Z

@sanderr What happens if we can't find the InmantaType? Do we default to Any?

jptrindade · 2025-01-21T09:10:23Z

tests/compiler/test_plugin_types.py

+    namespace = Namespace("dummy-namespace")
+    # namespace.set_primitives(inmanta_type.TYPES)
+    # FIXME: Not working    because of std = self.get_ns_from_string("std") std is None
+    namespace.primitives = inmanta_type.TYPES
+
+    location: Range = Range("test", 1, 1, 2, 1)
+
+    assert inmanta_type.String() == to_dsl_type(plugin_typing.string, location, namespace)
+
+    assert inmanta_type.NullableType(inmanta_type.Integer()) == to_dsl_type(
+        Annotated[int | None, "something"], location, namespace
+    )
+
+    assert inmanta_type.TypedDict(inmanta_type.Type()) == to_dsl_type(
+        Annotated[dict[str, int], plugin_typing.InmantaType("dict")], location, namespace
+    )
+
+    # FIXME: Not working
+    # entity: Entity = Entity("my-entity", namespace)
+    # namespace.define_type("my-entity", entity)
+    # assert inmanta_type.TypedDict(inmanta_type.Type()) == to_dsl_type(
+    #     Annotated[Union[int, str] | None, plugin_typing.InmantaType("my-entity")], location, namespace
+    # )


It is a bit awkward setting up the namespace in order to do this. Do you have any tips @sanderr ?

Make a method / fixture to_dsl_type_simple(python_type: str)?

jptrindade · 2025-01-21T09:12:46Z

src/inmanta/plugin_typing.py

I copied this file from your PR @sanderr . I'm inclined to leave it here. Regarding the comment on how to handle std::Entity I also think object might be our best option

sanderr · 2025-01-21T15:19:15Z

src/inmanta/plugin_typing.py

+import typing
+from dataclasses import dataclass
+
+# TODO: move this module to inmanta.plugins.typing? Probably not because that would import the whole `plugins` namespace?


I'd put it in plugins or plugins.typing. I don't recall why I was hesitant to put it there tbh. It's for annotating plugins, users would need to import it either way.

I think I'd prefer plugins.typing. @wouterdb may have an opinion.

plugins.typing would have the added difficulty that it's less trivial to import partially qualified.

The only thing awkward I see is the import confusion between typing and inmanta.plugins.typing in plugins.py.
I fixed this by importing everything we use instead of just doing import typing

I don't think that is a proper solution. qualified / unqualified should be driven by context to make sure that everything is unambiguous. Sure, things like Any and Sequence and such (though the latter should really come from collections.abc) are sufficiently wide-spread that they are unmabiguous in their own right. But get_args etc, those really require context. Imo we must use them qualified (qualifiedly?).

As to the awkwardness, I agree on that front. What I meant to say is that from inmanta.plugins import typing will never be a good approach. That leaves import inmanta.plugins.typing, which is very verbose, and import inmanta.plugins.typing; from inmanta import plugins (allowing partially qualified plugins.typing.InmantaType), which is a bit exotic, but we do it in a few places, and I don't think there's anything wrong with it myself.

So the question becomes, do we consider this import awkwardness sufficient reason to just add it to the heap of stuff in inmanta.plugins? I honestly don't know.

sanderr · 2025-01-21T15:19:30Z

src/inmanta/plugin_typing.py

+
+
+# TODO: how to do Entity? "object" is appropriate but raises too many errors for practical use. Any is Any
+Entity: typing.TypeAlias = typing.Annotated[object, InmantaType("std::Entity")]


This needs a docstring

sanderr · 2025-01-21T15:19:51Z

src/inmanta/plugin_typing.py

+
+# TODO: how to do Entity? "object" is appropriate but raises too many errors for practical use. Any is Any
+Entity: typing.TypeAlias = typing.Annotated[object, InmantaType("std::Entity")]
+string: typing.TypeAlias = typing.Annotated[str, InmantaType("string")]


I don't think this is useful anymore, considering that we support the native type now.

sanderr · 2025-01-21T15:20:37Z

src/inmanta/plugin_typing.py

+    evaluated as a DSL type, extended with "any".
+    For maximum static type coverage, it is recommended to use these only when absolutely necessary, and to use them as deeply
+    in the type as possible, e.g. prefer `Sequence[Annotated[MyEntity, InmantaType("std::Entity")]]` over
+    `Annotated[Sequence[Entity], InmantaType("std::Entity[]")]`.


Suggested change

`Annotated[Sequence[Entity], InmantaType("std::Entity[]")]`.

`Annotated[Sequence[MyEntity], InmantaType("std::Entity[]")]`.

sanderr · 2025-01-21T15:20:59Z

src/inmanta/plugin_typing.py

+    dsl_type: str
+
+
+# TODO: how to do Entity? "object" is appropriate but raises too many errors for practical use. Any is Any


We'll have to run this by Bart. I prefer object, as you propose, but I tend to be more strict than others.

sanderr · 2025-01-21T15:22:09Z

src/inmanta/plugins.py

-from inmanta import const, protocol, util
-from inmanta.ast import (  # noqa: F401 Plugin exception is part of the stable api
+from inmanta import const, plugin_typing, protocol, util
+from inmanta.ast import (  # noqa: F401 Plugin e


This looks like an accidental change to me.

sanderr · 2025-01-21T15:31:22Z

src/inmanta/plugins.py

-    #         return parse_dsl_type(inmanta_types[0].dsl_type)
-    #     # the annotation doesn't concern us => use base type
-    #     return to_dsl_type(args[0])
+    if typing.get_origin(python_type) is typing.Annotated:


I'd indent this one level under the is_generic body. I don't think it's strictly necessary, and there's something to say for not nesting deeper than required, but I think that in this case consistency is more important.

And then you can use origin instead of get_origin

sanderr · 2025-01-21T15:33:44Z

src/inmanta/plugins.py

+                raise TypingException(None, f"invalid type {python_type}, only one InmantaType annotation is supported")
+            return parse_dsl_type(inmanta_types[0].dsl_type, location, resolver)
+        # the annotation doesn't concern us => use base type
+        return to_dsl_type(annotated_args[0], location, resolver)


@sanderr What happens if we can't find the InmantaType? Do we default to Any?

If it's not there, it simply means that the Annotated has nothing to do with us. e.g. typing.Annotated[str, ContextForSomeMypyPlugin()]. So as far as our type-checking is concerned, it's just str. That's why we fall back to the base type here.

I don't think that is the case.
If I have Annotated[str, "something"] it will default to string.
But if I have Annotated[str, plugin_typing.InmantaType("unexistent-entity")] It should fail on parse_dsl_type.

I could either ignore the annotation and just treat it as str (even though it was clearly tagged as something that concerns us) or return Any (I think I prefer the other option).

But either way, if this happens we should log a warning

Oh, I misunderstood the question. I'd make sure to treat it the same as any other DSL-type plugin annotation. I would expect that means to raise an exception. i.e. what if you do def my_plugin(arg: "not-a-valid-type") -> None: ...?

sanderr · 2025-01-21T15:42:51Z

changelogs/unreleased/8573-add-support-for-annotated-types.yml

+description: Add support for annotated types to plugins.
+issue-nr: 8573
+change-type: minor
+destination-branches: [master, iso8]


We probably want to include this in iso7 as well. Discussion still pending, see Slack.

sanderr · 2025-01-21T15:43:37Z

tests/compiler/test_plugin_types.py

+# from inmanta.ast.entity import Entity
+# from inmanta.ast.type import TYPES
+


Testing artifact?

sanderr · 2025-01-21T15:45:22Z

tests/compiler/test_plugin_types.py

+    namespace = Namespace("dummy-namespace")
+    # namespace.set_primitives(inmanta_type.TYPES)
+    # FIXME: Not working    because of std = self.get_ns_from_string("std") std is None
+    namespace.primitives = inmanta_type.TYPES
+
+    location: Range = Range("test", 1, 1, 2, 1)
+
+    assert inmanta_type.String() == to_dsl_type(plugin_typing.string, location, namespace)
+
+    assert inmanta_type.NullableType(inmanta_type.Integer()) == to_dsl_type(
+        Annotated[int | None, "something"], location, namespace
+    )
+
+    assert inmanta_type.TypedDict(inmanta_type.Type()) == to_dsl_type(
+        Annotated[dict[str, int], plugin_typing.InmantaType("dict")], location, namespace
+    )
+
+    # FIXME: Not working
+    # entity: Entity = Entity("my-entity", namespace)
+    # namespace.define_type("my-entity", entity)
+    # assert inmanta_type.TypedDict(inmanta_type.Type()) == to_dsl_type(
+    #     Annotated[Union[int, str] | None, plugin_typing.InmantaType("my-entity")], location, namespace
+    # )


Make a method / fixture to_dsl_type_simple(python_type: str)?

sanderr · 2025-01-21T15:47:14Z

tests/compiler/test_plugin_types.py

+    # FIXME: Not working
+    # entity: Entity = Entity("my-entity", namespace)
+    # namespace.define_type("my-entity", entity)
+    # assert inmanta_type.TypedDict(inmanta_type.Type()) == to_dsl_type(
+    #     Annotated[Union[int, str] | None, plugin_typing.InmantaType("my-entity")], location, namespace
+    # )


I would reserve custom types for a test with a compile.

sanderr · 2025-01-21T15:47:52Z

tests/data/modules/plugin_native_types/model/_init.cf

@@ -0,0 +1,5 @@
+import std


This is not required. Though it doesn't hurt either.

sanderr · 2025-01-21T15:52:42Z

tests/compiler/test_plugins.py

+plugin_native_types::annotated_arg_entity(test_entity)     # type value: Annotated[object, InmantaType("TestEntity")]
+plugin_native_types::annotated_return_entity(test_entity)  # type return value: Annotated[object, InmantaType("TestEntity")]
+        """,
+        autostd=True,


Suggested change

autostd=True,

ministd=True,

I think this should suffice here. Less overhead.

sanderr · 2025-01-21T15:54:21Z

tests/data/modules/plugin_native_types/plugins/__init__.py

+
+
+@plugin
+def annotated_arg_entity(value: Annotated[object, plugin_typing.InmantaType("TestEntity")]) -> None:


I'd suggest trying with a python type that's not natively supported and/or one that is incompatible with the dsl type. i.e. object translates to any, meaning this would be valid even if we ignored InmantaType.

e.g. you could define a custom

class MyEntity(typing.Protocol): x: int

and then use Annotated[MyEntity, plugin_typing.InmantaType("TestEntity")]. That would tell us more, because without the annotated, it wouldn't be a valid type signature for a plugin.

sanderr · 2025-01-21T15:56:02Z

tests/compiler/test_plugins.py

@@ -435,6 +436,10 @@ def test_native_types(snippetcompiler: "SnippetCompilationTest") -> None:
 a = plugin_native_types::many_arguments(["a","c","b"], 1)

 none = plugin_native_types::as_none("a")
-        """
+        # Annotated types
+plugin_native_types::annotated_arg_entity(test_entity)     # type value: Annotated[object, InmantaType("TestEntity")]


I'd like to have an rejection scenario for this one as well. May require a dedicated test case.

sanderr · 2025-01-21T16:00:56Z

tests/data/modules/plugin_native_types/plugins/__init__.py

+
+
+@plugin
+def annotated_arg_entity(value: Annotated[object, plugin_typing.InmantaType("TestEntity")]) -> None:


Could you also add one that uses a typedef as its dsl type? e.g. a toy example of a valid use case might be

DSL: typedef response as string matching self in ["yes", "no"]
Python:

@plugin def process_response(response: Annotated[Literal["yes", "no"], InmantaType("response")]) -> None ...

…ue/8573-add-support-for-annotated-types-on-plugin # Conflicts: # src/inmanta/plugins/__init__.py # tests/compiler/test_plugin_types.py # tests/compiler/test_plugins.py # tests/data/modules/plugin_native_types/plugins/__init__.py

…8573-add-support-for-annotated-types-on-plugin

sanderr · 2025-01-23T10:51:36Z

tests/compiler/test_plugin_types.py


    # Check that a warning is produced when implicit cast to 'Any'
    caplog.clear()
-    to_dsl_type(complex, location, namespace)
+    to_dsl_type_simple(complex)


This one sparked a laugh

sanderr · 2025-01-23T12:36:18Z

src/inmanta/plugins/typing.py

+    dsl_type: str
+
+
+Entity: typing.TypeAlias = typing.Annotated[object, InmantaType("std::Entity")]


I still want to ask Bart's opinion on object vs Any here, but he's not available today. If we do go for object, I think it might be useful to also introduce Resource, for which we can introduce a typing.Protocol with the (read-only) fields that all resources have. i.e. requires, provides, receive_events (...?).

And PurgeableResource

Let's not implement those yet, until we know about the object vs Any, and get some input from the solutions team on whether these would actually be useful.

sanderr · 2025-01-23T16:34:56Z

changelogs/unreleased/8574-add-support-native-union-types.yml

Something weird seems to have happened here. Could you merge in master? That might make it all a lot easier to review.

Oh, nevermind, I see that I haven't merged the union PR yet. I went over all my PRs in the morning, but this is not officially mine so I missed it. I'll review like this, but it'll be for tomorrow, I didn't get where I'd hoped today.

This reverts commit b07e091.

…tated-types-on-plugin # Conflicts: # changelogs/unreleased/8574-add-support-native-union-types.yml # src/inmanta/plugins/__init__.py # tests/compiler/test_plugin_types.py # tests/compiler/test_plugins.py # tests/data/modules/plugin_native_types/plugins/__init__.py

arnaudsjs and others added 11 commits January 17, 2025 10:02

Add support for native union types in plugins

869603d

Add changelog entry

15129ab

Fix typing

e2c4c32

Fix typing

719fbb3

Small improvements

001f50c

Use callable from abc

c1c7df8

Improve error message

53a0a5c

Fix formatting

505c41e

Initial commit

fdeea80

small adjustments

23cd1e7

fixed mypy

2bc3eaa

jptrindade requested a review from sanderr January 21, 2025 08:54

test with type alias

8c30ca3

jptrindade commented Jan 21, 2025

View reviewed changes

jptrindade added 3 commits January 21, 2025 09:50

Update mypy-baseline.txt

bdf64cb

fix mypy

d1f7288

revert mypy

5ce1296

sanderr reviewed Jan 21, 2025

View reviewed changes

sanderr requested changes Jan 21, 2025

View reviewed changes

sanderr and others added 6 commits January 22, 2025 11:07

renamed exception

3bbe231

dedicated plugin type exception

ec784bf

added test scenario

c16d8b4

import fix

dac146d

addressed comments

f64c3a9

added to_dsl_type_simple

db3c27e

jptrindade self-assigned this Jan 22, 2025

jptrindade and others added 3 commits January 22, 2025 11:31

fix mypy

378ca58

some test fixes

0127969

test fixes

092261b

sanderr and others added 2 commits January 22, 2025 15:54

pep8

6a6d6ec

rejection test first try

28e62aa

jptrindade requested a review from sanderr January 22, 2025 15:13

jptrindade changed the title ~~Draft: Add support for Annotated types in plugins~~ Add support for Annotated types in plugins Jan 22, 2025

sanderr and others added 4 commits January 22, 2025 16:15

renamed change entry

69f3697

include iso7

1fdd946

merge and refactor

b07e091

jptrindade changed the base branch from master to issue/8574-add-support-union-types January 23, 2025 16:13

jptrindade changed the base branch from issue/8574-add-support-union-types to master January 23, 2025 16:13

jptrindade added 2 commits January 23, 2025 16:16

mypy

2a1a57d

Merge remote-tracking branch 'refs/remotes/origin/master' into issue/…

3484f4a

…8573-add-support-for-annotated-types-on-plugin

sanderr reviewed Jan 23, 2025

View reviewed changes

jptrindade added 5 commits January 23, 2025 16:36

Revert "merge and refactor"

0b21043

This reverts commit b07e091.

attempted merge

9e5679e

fixed mypy

2fc91a9

fixed test_plugin_types.py

48a1a97

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Annotated types in plugins #8665

Add support for Annotated types in plugins #8665

jptrindade commented Jan 20, 2025 •

edited

Loading

jptrindade commented Jan 21, 2025

jptrindade Jan 21, 2025

sanderr Jan 21, 2025

jptrindade Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

jptrindade Jan 22, 2025

sanderr Jan 22, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

jptrindade Jan 21, 2025

jptrindade Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 21, 2025

sanderr Jan 23, 2025

sanderr Jan 23, 2025

sanderr Jan 23, 2025

sanderr Jan 23, 2025

sanderr Jan 23, 2025

sanderr Jan 23, 2025



		# TODO: how to do Entity? "object" is appropriate but raises too many errors for practical use. Any is Any
		Entity: typing.TypeAlias = typing.Annotated[object, InmantaType("std::Entity")]

	`Annotated[Sequence[Entity], InmantaType("std::Entity[]")]`.
	`Annotated[Sequence[MyEntity], InmantaType("std::Entity[]")]`.

		dsl_type: str


		# TODO: how to do Entity? "object" is appropriate but raises too many errors for practical use. Any is Any

		# from inmanta.ast.entity import Entity
		# from inmanta.ast.type import TYPES



		@plugin
		def annotated_arg_entity(value: Annotated[object, plugin_typing.InmantaType("TestEntity")]) -> None:

Add support for Annotated types in plugins #8665

Are you sure you want to change the base?

Add support for Annotated types in plugins #8665

Conversation

jptrindade commented Jan 20, 2025 • edited Loading

Description

Self Check:

jptrindade commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jptrindade commented Jan 20, 2025 •

edited

Loading