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.

Sign up for GitHub

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

Jump to bottom

Feat: Allow macros in python model properties #3740

Open

themisvaltinos wants to merge 6 commits into main from themis/enabled_python_macros

+297 −29

Contributor

themisvaltinos commented Jan 29, 2025 •

edited

Loading

This update enables macros to be passed in the properties for Python models, aside from audits, signals, merge filter and physical_properties which are rendered at evaluation time (the latter will be addressed on a subsequent pr). This allows to dynamically control for example the enabled flag based on the gateway:

@model(
  ...,
  enabled="@IF(@gateway = 'local', True, False)",
)

themisvaltinos requested a review from a team

January 29, 2025 16:42

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/decorator.py Outdated

+                                  )
+                              )
+                      renderer_kwargs = {

Member

izeigerman Jan 29, 2025

How was this set compiled? Can we have this set shared somehow with the one in load_sql_based_model? I don't want to update one place and then forget about the other one

Contributor Author

themisvaltinos Jan 29, 2025

right I will see how to either share or refactor to have a function with the same logic in both load_sql_based_model and here

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/decorator.py Outdated

+                          "dialect": dialect,
+                          "default_catalog": default_catalog,
+                      }
+                      meta_renderer = _meta_renderer(

Member

izeigerman Jan 29, 2025

Perhaps, instead of kwargs, the _meta_renderer function should take all the arguments above by name explicitly (no default values).

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/decorator.py Outdated

@@ @@ -119,21 +121,49 @@ def model( @@
                       build_env(self.func, env=env, name=entrypoint, path=module_path)
+                      # Properties to be rendered at model creation time
+                      expressions = [exp.Property(this="name", value=exp.to_table(self.name, dialect=dialect))]
+                      for field_name in {"enabled", "start", "end"}:

Member

izeigerman Jan 29, 2025

Why only these properties? Shouldn't we instead take all fields from the ModelMeta schema with few exceptions like signals?

Contributor Author

themisvaltinos Jan 29, 2025

ok I will add all fields of ModelMeta instead, aside from these that shouldn't be rendered at creation time. it will require specific handling for each property, but it can be done having something similar to META_FIELD_CONVERTER like in render_definition

georgesittas reviewed

View reviewed changes

sqlmesh/core/model/decorator.py Outdated

               from sqlmesh.core.model.definition import (
                   Model,
+                  _meta_renderer,

Contributor

georgesittas Jan 29, 2025

Should we rename this to get rid of the leading _? It's not a private helper anymore if we reference it here.

sqlmesh/core/model/decorator.py Outdated

Comment on lines 129 to 131

		and isinstance(field_value, exp.Expression)
		or (isinstance(field_value, str) and field_value.startswith(SQLMESH_MACRO_PREFIX))

Contributor

georgesittas Jan 29, 2025

Let's include parentheses so that operator precedence can be easily understood.

sqlmesh/core/model/decorator.py Outdated Show resolved Hide resolved

tobymao reviewed

View reviewed changes

sqlmesh/core/model/decorator.py Outdated

@@ @@ -119,21 +121,49 @@ def model( @@
                       build_env(self.func, env=env, name=entrypoint, path=module_path)
+                      # Properties to be rendered at model creation time
+                      expressions = [exp.Property(this="name", value=exp.to_table(self.name, dialect=dialect))]

Contributor

tobymao Jan 29, 2025

"name" should be a var, otherwise the ast is invalid

themisvaltinos force-pushed the themis/enabled_python_macros branch 2 times, most recently from 8065b51 to 7cf4024 Compare

January 31, 2025 15:31

Contributor Author

themisvaltinos commented Jan 31, 2025

Thanks for the review @izeigerman , @georgesittas , @tobymao addressed comments and revised logic to handle all model properties, except for those that need to be rendered at evaluation time

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py

+                  fields: t.Dict[str, t.Any],
+                  module_path: Path,
+                  path: Path,
+                  jinja_macros: t.Optional[JinjaMacroRegistry] = None,

Member

izeigerman Feb 1, 2025

I suggest we don't have default values for any fields here, so that we know to explicitly provide them everywhere we call this

Contributor Author

themisvaltinos Feb 2, 2025

The issue is they flow in as optional from model's arguments:

sqlmesh/sqlmesh/core/model/decorator.py

Lines 87 to 90 in dc613d0

    
           macros: t.Optional[MacroRegistry] = None, 
        
           jinja_macros: t.Optional[JinjaMacroRegistry] = None, 
        
           audit_definitions: t.Optional[t.Dict[str, ModelAudit]] = None, 
        
           dialect: t.Optional[str] = None,

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                                  default_catalog=default_catalog,
+                              )
+                              assert rendered_expr and len(rendered_expr) == 1
+                              return rendered_expr[0].sql(dialect=dialect)

Member

izeigerman Feb 1, 2025

This doesn't look right. If the input is expression, we return a string instead?

Member

izeigerman Feb 1, 2025

Shouldn't we always return an expression?

Contributor Author

themisvaltinos Feb 2, 2025

Yes revised it, this was needed for the model's name, but adjusted it for this field only

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                              assert rendered_expr and len(rendered_expr) == 1
+                              return rendered_expr[0].sql(dialect=dialect)
+                          except Exception:
+                              pass

Member

izeigerman Feb 1, 2025

Why ignore?

Contributor Author

themisvaltinos Feb 2, 2025

This was for cron that it was for example trying to render and failing to find @daily macro, but revised it and simply added cron in the RUNTIME_RENDERED_MODEL_FIELDS as well.

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                      return value
+                  for field_name, _ in ModelMeta.all_field_infos().items():
+                      field_name = field_name[:-1] if field_name.endswith("_") else field_name

Member

izeigerman Feb 1, 2025

No need to do this. You can get the alias from the FieldInfo object in the value of the dict that you're iterating over.

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                  for field_name, _ in ModelMeta.all_field_infos().items():
+                      field_name = field_name[:-1] if field_name.endswith("_") else field_name
+                      if (field_value := fields.get(field_name)) and field_name not in {
+                          "audits",

Member

izeigerman Feb 1, 2025

Should we set these in a named constant like RUNTIME_RENDERED_MODEL_FIELDS

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                          "virtual_properties",
+                          "session_properties",
+                      }:
+                          if isinstance(field_value, t.Dict):

Member

izeigerman Feb 1, 2025

Hm, not sure whether it's a good practice to check for instance of t.Dict. I believe we just check for dict in other places.

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                      }:
+                          if isinstance(field_value, t.Dict):
+                              for key, value in field_value.items():
+                                  if key != "merge_filter":

Member

izeigerman Feb 1, 2025

Can be a part of the same RUNTIME_RENDERED_MODEL_FIELDS

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                              for key, value in field_value.items():
+                                  if key != "merge_filter":
+                                      fields[field_name][key] = render_field_value(value)
+                          elif isinstance(field_value, t.List):

Member

izeigerman Feb 1, 2025

Ditto: isinstance(..., list)

izeigerman reviewed

View reviewed changes

sqlmesh/core/model/definition.py Outdated

+                                  dialect=dialect,
+                                  default_catalog=default_catalog,
+                              )
+                              assert rendered_expr and len(rendered_expr) == 1

Member

izeigerman Feb 1, 2025

Should we inform the user about the problem here?

Contributor Author

themisvaltinos Feb 2, 2025

Right adapted it to follow instead the same pattern we have in the rest of the places

themisvaltinos force-pushed the themis/enabled_python_macros branch from 7cf4024 to 4432204 Compare

February 2, 2025 09:56

themisvaltinos requested a review from a team

February 3, 2025 08:18

themisvaltinos added 5 commits

February 3, 2025 13:05


          Feat: Allow passing macros to python model properties

fbd0606


          Refactors

50f98d1


          Adapt to handle all model meta properties; bar those that are rendere…

d7fb59b

…d later


          Refactors and code cleanup

02f5381


          Add unit tests for rest of the properties; don't render description

254fe8b

themisvaltinos force-pushed the themis/enabled_python_macros branch from 4432204 to 254fe8b Compare

February 3, 2025 11:10


          Add virtual and session properties

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet