Skip to content

Fix: parse runtime-rendered fields, extract python env deps from merge_filter #4905

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

georgesittas
Copy link
Contributor

While iterating on a separate task locally, I observed that there are a couple of gaps in our "runtime-rendered" property handling logic:

  • We don't parse Python strings corresponding to said properties. This means we don't analyze them to extract referenced variables, which can lead to issues at runtime (e.g., cadence runs), due to these variables not being present in python_env, and hence failing to render.
  • We don't analyze merge_filter for variables. This is true for both Python and SQL models. Again, this means that if a variable is referenced in merge_filter, it may not be present in the python_env of the model and hence we may fail to render it at runtime.

This PR addresses both of the issues above and expands our testing coverage to ensure we properly detect the referenced variables in runtime-rendered properties.

I'm not sure yet if this needs a migration script, because if someone references variables in said properties today, then cadence runs would've failed for them anyway if the variables weren't present in the python_env. I'd like some eyes on this to double-check my reasoning.

@georgesittas georgesittas requested review from izeigerman, themisvaltinos and a team July 4, 2025 11:56
Comment on lines -80 to -81
"description",
"cron",
Copy link
Contributor Author

@georgesittas georgesittas Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were added some time ago to avoid parsing & rendering things like cron="@daily", or description="... @bar ...", where @bar is just a literal substring of the description, not a variable reference.

I'm not sure if this is the right approach, because we may have more fields with "string literals", where we don't care about rendering variables. I decided to simply preserve what we do today to avoid increasing the scope, but this is probably worth discussing & addressing in a followup PR.

Copy link
Contributor

@themisvaltinos themisvaltinos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, the only concern is regarding the need of a migration script, but I think your theory is correct, since if someone had macro vars in the merge_filter before they would have failed to be rendered

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.

2 participants