Skip to content

Conversation

@ravenac95
Copy link
Member

@ravenac95 ravenac95 commented Aug 18, 2025

  • Adds testing for for this scenario
  • Enhances the metadata returned from the materialization (this also helped with validating the testing)

First part of opensource-observer/oso#4757

The follow on would be to update the dependency in the oso repo.

A bug was found that if a plan was empty.
Also adds more metadata for each asset
@ravenac95 ravenac95 force-pushed the ravenac95/run-should-trigger-materialization-result branch from e371e61 to 863ebac Compare August 18, 2025 22:50
Comment on lines +257 to +262
if model_name_for_notification in self._non_model_names:
self._current_index += 1
self.logger.debug(
f"skipping non-model snapshot {model_name_for_notification}"
)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this the bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh. Sadly the bug was a bit multi-faceted.

This was part of the issue, but generally, the bug was that we created a queue of things to be reported as materialized. We did this because if you enable subsetting of the dagster asset you need to avoid out of order AssetMaterialization objects being yielded from the asset. So the logic to initialize that set of things was all funky. The core of the problems were in the Materialization tracker, which maybe could require further rethinking, but I think we now get a fuller picture of what's happening with these changes.

@ravenac95 ravenac95 added this pull request to the merge queue Aug 18, 2025
Merged via the queue into main with commit ea62863 Aug 18, 2025
2 checks passed
@ravenac95 ravenac95 deleted the ravenac95/run-should-trigger-materialization-result branch August 18, 2025 23:10
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.

3 participants