remove get_event_records calls from multi asset sensor #21784
Conversation
|
|
||
| return events | ||
| return self.instance.fetch_materializations( |
There was a problem hiding this comment.
not blocking, but there's probably a way to avoid duplicate code here, e.g. by tracking the total number of records we've fetched so far across all iterations and seeing when it reaches the user-provided limit
There was a problem hiding this comment.
yeah... this is the PR that I think could be rewritten the most. I haven't quite grokked all the fetching patterns - it feels like there's a better way to do this.
Also, it feels like we should revisit some of the APIs that we expose here to have better data fetching hygiene. Probably not the highest priority item, but if you had time to pair on this, I'd definitely be willing.
There was a problem hiding this comment.
I wouldn't be surprised if we remove multi-asset sensor after auto-materialize policies support user-defined functions
There was a problem hiding this comment.
(Which is to say I think it's probably not worth putting too much effort here)
## Summary & Motivation We want to deprecate get_event_records calls in favor of narrower APIs. This call-site has the most user-facing changes in converting from `get_event_records` -> `fetch_materializations`, mostly because we have to enforce a limit on a sensor method that we expose off of the context. I'm not sure if there's a more elegant way to do this, based on the internals of the multi assent sensor, and knowing how many records we can advance per tick (I have a sneaking suspicion that we can do something more clever here). This PR takes the crudest possible approach: batching calls for chunks in a loop, and warning when called without a limit. ## How I Tested These Changes BK
Summary & Motivation
We want to deprecate get_event_records calls in favor of narrower APIs.
This call-site has the most user-facing changes in converting from
get_event_records->fetch_materializations, mostly because we have to enforce a limit on a sensor method that we expose off of the context.I'm not sure if there's a more elegant way to do this, based on the internals of the multi assent sensor, and knowing how many records we can advance per tick (I have a sneaking suspicion that we can do something more clever here).
This PR takes the crudest possible approach: batching calls for chunks in a loop, and warning when called without a limit.
How I Tested These Changes
BK