-
Notifications
You must be signed in to change notification settings - Fork 25
Fix computed attribute setup to only update each computed attribute once #6402
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
Conversation
changelog/+removeschematextfields.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not related to this PR but I noticed these 2 files that don't have the right format in the changelog directory
CodSpeed Performance ReportMerging #6402 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I like the idea of storing some kind of hash "somewhere". We'd have to consider something for the Python transform based ones the for Jinja2 the hash alone would do, if we can get the commit ID for the repo of the transforms we could include that too. We have the description field within the transform itself, other than that it could be something in our database.
# Because the automation in Prefect doesn't capture all information about the computed attribute | ||
# we can't tell right now if a given computed attribute has changed and need to be updated | ||
unique_nodes: set[tuple[str, str, str]] = { | ||
(trigger.branch, trigger.computed_attribute.kind, trigger.computed_attribute.attribute.name) # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mypy ignore actually required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch ... thks
b528160
to
39ed671
Compare
While working on #6393, I noticed a bug with
computed_attribute_setup_jinja2
that cause some computed attribute to be updated multiple time after the schema has been updated, if they are referencing more than one kind of nodes.Looking closer at the code, I tried to see if I could update this part of the code to avoid updating all computed attributes when we update the schema.
As part of this effort, I've refactored the function
setup_triggers
to be idempotent and to properly report which automations has been updated instead of updating everything all the time. This by itself isn't a complete fix but if we could generate a hash that represent the part of the computed attribute that isn't captured in the Automation and include it somehow in the automation, I think it would provide a reliable way to identify which computed attribute needs to be recalculated.