Skip to content
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

Tool for generating schema type annotations #7186

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aljazerzen
Copy link
Contributor

@aljazerzen aljazerzen commented Apr 12, 2024

Our schema classes have generated getter methods for each of the fields:

class Pointer:
    source = so.SchemaField(
        so.InheritingObject,
        default=None, compcoef=None,
        inheritable=False
    )
 

    # this is auto generated by a meta class in objects.py
    def get_source(schema: s_schema.Schema) -> objects.InheritingObject: ...

This PR adds a tool that generates .pyi files in edb/schema/, which contain definitions for all of these generated getters.


We do have mypy extension that already picks-up these getters, even without .pyi definitions.

Regardless, this .pyi approach is useful because language servers (PyLance) cannot use mypy extension. Now, "go to definition", "show usages" and other code actions work.

There is a few drawbacks:

  • PyLance will not look at .pyi file when editing associated .py file. They suggest merging the two files.
  • In this example:
    subjexpr = constr.get_subjectexpr(ctx.env.schema)
    ... PyLance will understand that get_subjectexpr returns Expression, but it will infer type of subjexpr to be Any. What?

Because of these drawbacks, #7166 is still useful.

@aljazerzen aljazerzen changed the title schema type annotations Tool for generating schema type annotations Apr 12, 2024
@aljazerzen aljazerzen requested review from msullivan and dnwpark April 12, 2024 12:00
@aljazerzen
Copy link
Contributor Author

The verdict here is:

  • we want this, but working better. This would best be done with these .pyi files being actual
    .py files in edb/schema/generated/ and the base classes adding the generated type-annotation classes as a mixin.

    # edb/schema/pointers.py
    
    class Pointer(..., PointerMixin):
        source = so.SchemaField(
          so.InheritingObject,
          default=None, compcoef=None,
          inheritable=False
        )
    
    # edb/schema/generated/pointers.py
    class PointerMixin:
        def get_source(schema: Schema) -> so.InheritingObject: ...
  • alternatively, we could stop creating these functions at run-time and just generate them in the mixin classes:

    # edb/schema/pointers.py
    
    class Pointer(..., PointerMixin):
        source = so.SchemaField(
          so.InheritingObject,
          default=None, compcoef=None,
          inheritable=False
        )
    
    # edb/schema/generated/pointers.py
    class PointerMixin:
        def get_source(self, schema: Schema) -> so.InheritingObject:
            self.get_field_value('source', schema)

@aljazerzen aljazerzen force-pushed the schema-type-annotations branch from 133c311 to a1ca831 Compare April 30, 2024 10:20
@aljazerzen aljazerzen force-pushed the schema-type-annotations branch from ae8475b to 01a40c4 Compare August 1, 2024 07:39
@aljazerzen
Copy link
Contributor Author

@msullivan, can you have a look into why mypy is failing? A new realization is that that if I disable our plugin, the internal error disappears. I've also figured that if I disable StructTransformer, it disappears:

https://github.com/edgedb/edgedb/blob/schema-type-annotations/edb/tools/mypy/plugin.py#L73-L82

@aljazerzen
Copy link
Contributor Author

aljazerzen commented Aug 1, 2024

I've tested out performance impact of this change. Currently, there are a few commits on this PR, starting with the most basic setup and then optimizing from there:

git ref compile_edgeql_script_webapp_get_movie compared to master
master 74.8 ms += 2.1 ms non-significant change
schema-mixins 93.4 ms += 3.4 ms 24.87% SLOWER
getter-specialization 84.3 ms += 2.9 ms 12.66% SLOWER
inlined-getters 79.7 ms += 3.0 ms 6.52% SLOWER
static-field 75.3 ms += 1.6 ms non-significant change

The static-field approach does generate a lot of code, which could probably be cleaned-up a little without a major impact on performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant