Native python types #8559

wouterdb · 2025-01-06T13:16:18Z

Description

Basic native python types

For the documentation, I would add a separate ticket

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)

src/inmanta/plugins.py

sanderr · 2025-01-06T13:18:35Z

src/inmanta/plugins.py

+python_to_model = {
+    str: inmanta_type.String(),
+    float: inmanta_type.Float(),
+    numbers.Number: inmanta_type.Number(),


Do we want to support this, given that the number type has been deprecated?

Deprecated, but not gone.

I have no strong opinion on this

After giving it some more thought, me neither. At some level I'd prefer not to integrate deprecated types into new features, but if it's as simple as this I guess it doesn't hurt too much either. The only thing is perhaps that we have the opportunity to make a clean cut here, to be more visible than the current deprecation warning.

sanderr · 2025-01-06T13:28:44Z

src/inmanta/plugins.py

@@ -267,11 +345,13 @@ def resolve_type(self, plugin: "Plugin", resolver: Namespace) -> inmanta_type.Ty
            return self._resolved_type

        if not isinstance(self.type_expression, str):


Ok, so for iso 8.1 we also aim to evaluate strings here, right?

I had understood that every string is considered to be an inmanta type

I think I would prefer to also support python type strings, because that's what you would expect when writing Python. But that doesn't have to be now necessarily. What I had in mind was to

try to evaluate the annotation as a python type expression (converting any strings, even nested ones, to types)

convert python type expression to dsl type expression

If 1 fails -> fall back to dsl type parsing.

But I don't feel too strongly about this.

I don't want that. That would create the same type of problem that yaml has (is 00:00:00:00 a number?), that parsing outcome depends on context you (often) can't know. if a type 'string' is somehow in scope, the typing changes

Perhaps a key element in my reasoning is that I approach it from the idea that the pure dsl annotations would eventually disappear, leaving only the typing.Annotated when you really need them. With that reasoning, the Python parsing would always be the primary, and the DSL parsing would become only a backwards compatibility fallback.

But if that's not how you see it evolve, then perhaps I agree with you, given your argument above. I still think it can have unintuitive aspects of its own, but the typing.Annotated should give us a way to work around any we encounter to give ourselves time to discover where we really want to go.

sanderr · 2025-01-06T13:33:20Z

src/inmanta/plugins.py

+    str: inmanta_type.String(),
+    float: inmanta_type.Float(),
+    numbers.Number: inmanta_type.Number(),
+    int: inmanta_type.Integer(),
+    bool: inmanta_type.Bool(),
+    dict: inmanta_type.LiteralDict(),
+    list: inmanta_type.LiteralList(),


I'm actually not sure about hardcoding this mapping to the type objects. I think I'd prefer to use the existing inmanta.ast.type.TYPES mapping, so that if we ever change something there it is reflected in both flows. Then this mapping here would simply be a mapping to the model types as represented in the model, e.g. str -> "string".

Upon closer consideration, what I had here was even wrong.

inmanta.ast.type.TYPES is the internal attribute types, which are subtly different from what is here, mapping it twice won't be correct / consistent with what existed already.

I'm not following with this. From your comment I understood that you agreed we should use inmanta.ast.type.TYPES, but from the implementation I think I misunderstood? Why should we not use it?

I don't think we should use inmanta.ast.type.TYPES because:

I consider the type object the interface, so we should decouple on that

inmanta.ast.type.TYPES maps attribute type string to inmanta types, but this is subtly different from the type mapping at the plugin boundary. E.g. "dict": LiteralDict(), Vs dict: inmanta_type.TypedDict(inmanta_type.Type()), And this is a pre-existing condition

src/inmanta/plugins.py

sanderr · 2025-01-06T13:40:27Z

src/inmanta/plugins.py

+            if len(other_types) == 1:
+                return inmanta_type.NullableType(to_dsl_type(other_types[0]))
+            # TODO: optional unions
+            return inmanta_type.AnyType()


So we create a follow-up ticket to fix all these any types by 8.1? They could cause some very unintuitive behavior I think.

Then again, aren't these relatively easy to handle?

I will ticketize and we'll see when we get them.

(e.g. the unions could be easy, but then again, we don't use unions anywhere yet, so they could also be very difficult)

Can you mark the tickets for the 8.1 release? I don't want to keep all these Any's for longer than required, so I'd like to address it (on iso8) while we can still consider it a non-breaking change (because it's in there now but not documented).

src/inmanta/plugins.py

sanderr · 2025-01-06T13:59:34Z

src/inmanta/plugins.py

+
+    if typing_inspect.is_generic_type(python_type):
+        origin = typing.get_origin(python_type)
+        if  origin is Sequence:


From the design document: "Collection and Sequence as return types will require minor changes to DynamicProxy.unwrap, which now contains isinstance(item, list)" The same might go for Mapping, I haven't checked.

I think this is still missing.

src/inmanta/plugins.py

Co-authored-by: Bart Vanbrabant <[email protected]>

…into issue/native_types

src/inmanta/plugins.py

src/inmanta/ast/type.py

src/inmanta/plugins.py

Co-authored-by: Sander Van Balen <[email protected]>

…into issue/native_types

sanderr · 2025-01-07T10:37:46Z

src/inmanta/plugins.py

+    Mapping: inmanta_type.TypedDict(inmanta_type.Type()),
+    collections.abc.Mapping: inmanta_type.TypedDict(inmanta_type.Type()),


Something weird is going on here. Mapping should be from collections.abc. I think it's good to support both, but then the qualification must be the other way around (or both qualified if you prefer). Because it's very likely that the import will be updated at some point.

Same for sequence.

sanderr · 2025-01-07T10:39:15Z

src/inmanta/plugins.py

-            if not args:
-                return inmanta_type.List()
-            return inmanta_type.TypedList(to_dsl_type(args[0]))
+            if origin in [collections.abc.Sequence, list, typing.Sequence]:


Not Collection?

Collection also contains Mapping and Set, so I think that is a bit too wide?

But we're not accepting subclasses anymore, only those exact annotations. A list is a collection, so that would be acceptable for input args. And we can easily construct lists from it so it's acceptable as return type.

But I'm not too attached to it, it's not that common.

sanderr · 2025-01-07T10:40:23Z

src/inmanta/plugins.py

-            return inmanta_type.TypedDict(to_dsl_type(args[1]))
+                return inmanta_type.TypedDict(to_dsl_type(args[1]))
+            else:
+                raise TypingException(None, f"invalid type {python_type}, dictionary types should be Mapping or dict")


Nice. I hadn't considered this, but it's good to have a clear message!

One suggestion

Suggested change

raise TypingException(None, f"invalid type {python_type}, dictionary types should be Mapping or dict")

raise TypingException(None, f"invalid type {python_type}, dictionary types should be Mapping (preferred) or dict")

I think the order implies preference?

I guess. I mostly meant to say that I think we should discourage the dict, even if we accept it, because it's simply not accurate.

We could even not mention the dict, but it is the most familiar to most people

sanderr · 2025-01-07T14:07:45Z

@wouterdb FYI, there are still some threads that I consider relevant. You may have just skipped over them for now, but if you missed them this should bring them to your attention. I resolved all threads that I consider no longer relevant. Some of the others are only really relevant for follow-up tickets (e.g. whether or not we want to support stringified Python type expressions), others are decisions that need to be made (e.g. typing.Annotated now or not), and some are things that I'm just confused about now.

sanderr

Looks like we're starting to align on a middle ground. One final note on something that I'd prefer to see differently, but I can let it slide if you disagree.

sanderr · 2025-01-07T14:54:23Z

src/inmanta/plugins.py

+    if python_type in python_to_model:
+        return python_to_model[python_type]
+
+    return inmanta_type.Type()


I'm not a fan of this fallback to Any. This may easily give the user the false impression that they're getting type validation, while that is not actually the case.

See my proposed extensions on slack

inmantaci · 2025-01-07T15:14:06Z

Processing this pull request

inmantaci · 2025-01-07T15:14:08Z

Failed to merge changes into iso7 due to merge conflict. Please open a pull request for these branches separately by cherry-picking the commit that was made on the branch master (git cherry-pick 881d1d5).

inmantaci · 2025-01-07T15:14:12Z

Merged into branches master in 881d1d5

# Description Basic native python types For the documentation, I would add a separate ticket # Self Check: Strike through any lines that are not applicable (`~~line~~`) then check the box - [x] Attached issue to pull request - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] 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](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)

inmantaci · 2025-01-07T15:14:13Z

Not closing this pull request due to previously commented issues for some of the destination branches. Please open a separate pull request for those branches by cherry-picking the relevant commit. You can safely close this pull request and delete the source branch.

inmantaci · 2025-01-07T15:14:33Z

This branch was not deleted as it seems to still be in use.

Basic native python types For the documentation, I would add a separate ticket Strike through any lines that are not applicable (`~~line~~`) then check the box - [x] Attached issue to pull request - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] 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](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)

Stage 2 for #8559

Native python types

2a8177c

wouterdb requested review from bartv and sanderr January 6, 2025 13:16

wouterdb commented Jan 6, 2025

View reviewed changes

src/inmanta/plugins.py Outdated Show resolved Hide resolved

Added tests

708d8f2

wouterdb marked this pull request as ready for review January 6, 2025 13:40

Cleanup

a5a4f6e

sanderr requested changes Jan 6, 2025

View reviewed changes

bartv reviewed Jan 6, 2025

View reviewed changes

src/inmanta/plugins.py Show resolved Hide resolved

src/inmanta/plugins.py Outdated Show resolved Hide resolved

Fixes

eeb132d

wouterdb requested a review from sanderr January 6, 2025 14:53

wouterdb and others added 3 commits January 6, 2025 15:54

Update src/inmanta/plugins.py

61f0984

Co-authored-by: Bart Vanbrabant <[email protected]>

Fix pep8

6d6efc1

Merge branch 'issue/native_types' of github.com:inmanta/inmanta-core …

2daf160

…into issue/native_types

sanderr requested changes Jan 6, 2025

View reviewed changes

src/inmanta/plugins.py Outdated Show resolved Hide resolved

wouterdb added 3 commits January 7, 2025 09:13

Fix eq and hash

8bd58d3

Fixes

cc4d87a

fixes

bfd4e55

wouterdb requested a review from sanderr January 7, 2025 09:00

sanderr requested changes Jan 7, 2025

View reviewed changes

src/inmanta/ast/type.py Outdated Show resolved Hide resolved

src/inmanta/plugins.py Show resolved Hide resolved

wouterdb and others added 4 commits January 7, 2025 10:41

Update src/inmanta/ast/type.py

94ac77f

Co-authored-by: Sander Van Balen <[email protected]>

more tests

6d61bbd

Merge branch 'issue/native_types' of github.com:inmanta/inmanta-core …

6f71a84

…into issue/native_types

Restrict list types

d430ed2

sanderr requested changes Jan 7, 2025

View reviewed changes

change imports

2aa2eb9

wouterdb requested a review from sanderr January 7, 2025 14:39

sanderr requested changes Jan 7, 2025

View reviewed changes

sanderr approved these changes Jan 7, 2025

View reviewed changes

wouterdb added the merge-tool-ready This ticket is ready to be merged in label Jan 7, 2025

inmantaci removed the merge-tool-ready This ticket is ready to be merged in label Jan 7, 2025

inmantaci assigned wouterdb Jan 7, 2025

wouterdb mentioned this pull request Jan 7, 2025

Issue/stage 2 for 8559 #8578

Closed

wouterdb closed this Jan 7, 2025

wouterdb deleted the issue/native_types branch January 8, 2025 08:28

inmantaci pushed a commit that referenced this pull request Jan 8, 2025

Added support for python types in plugin annotations (PR #8578)

7a53c98

Stage 2 for #8559

Hugo-Inmanta mentioned this pull request Jan 16, 2025

native types: warn for implict conversion to any #8576

Closed

		@@ -267,11 +345,13 @@ def resolve_type(self, plugin: "Plugin", resolver: Namespace) -> inmanta_type.Ty
		return self._resolved_type

		if not isinstance(self.type_expression, str):

		Mapping: inmanta_type.TypedDict(inmanta_type.Type()),
		collections.abc.Mapping: inmanta_type.TypedDict(inmanta_type.Type()),

	raise TypingException(None, f"invalid type {python_type}, dictionary types should be Mapping or dict")
	raise TypingException(None, f"invalid type {python_type}, dictionary types should be Mapping (preferred) or dict")

Native python types #8559

Native python types #8559

Conversation

wouterdb commented Jan 6, 2025 • edited Loading

Description

Self Check:

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

wouterdb Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterdb Jan 7, 2025 • edited Loading

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

sanderr commented Jan 7, 2025

sanderr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inmantaci commented Jan 7, 2025

inmantaci commented Jan 7, 2025

inmantaci commented Jan 7, 2025

inmantaci commented Jan 7, 2025

inmantaci commented Jan 7, 2025

wouterdb commented Jan 6, 2025 •

edited

Loading

wouterdb Jan 6, 2025 •

edited

Loading

wouterdb Jan 7, 2025 •

edited

Loading