-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[DS][36/n] Create ScheduledSince condition #21712
Conversation
77c8447
to
bcdc712
Compare
594d77c
to
88dce77
Compare
bcdc712
to
d6b9e0e
Compare
88dce77
to
bde7a62
Compare
d6b9e0e
to
0d35118
Compare
0fd4a9d
to
1bbbd9a
Compare
1bbbd9a
to
3174f9a
Compare
4946a2f
to
eb565e8
Compare
3174f9a
to
6dae5df
Compare
eb565e8
to
b9a986c
Compare
6dae5df
to
c9aee95
Compare
b9a986c
to
97839e1
Compare
c9aee95
to
8ede4ae
Compare
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.
Awesome. Let some comments about a yet-to-be written abstraction in this layer, but we can write that later and change these callsites.
slices_with_metadata = [ | ||
AssetSliceWithMetadata( | ||
asset_slice.compute_difference(context.previous_requested_slice), metadata | ||
) | ||
for asset_slice, metadata in previous_slices_with_metadata | ||
] |
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 definitely speaks to me to the the need of a generalized abstraction encapsulate a set of slices with metadata we can operate. I was calling this a "space" in early prototypes which wasn't quire right
but
space.difference_with(context.previous_requested_slice)
would be very nice.
return [ | ||
asset_slice_with_metadata | ||
for asset_slice_with_metadata in slices_with_metadata | ||
if not ( | ||
asset_slice_with_metadata.asset_slice.is_empty | ||
or cast( | ||
float, | ||
asset_slice_with_metadata.metadata.get( | ||
_REQUEST_TIMESTAMP_METADATA_KEY, MetadataValue.float(0) | ||
).value, | ||
) | ||
< self._get_minimum_timestamp(context) | ||
) | ||
] |
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.
Similarly here this feels like it should be a primitive on a slightly higher level of abstraction.
97839e1
to
fc4c2a0
Compare
8ede4ae
to
5823a56
Compare
fc4c2a0
to
a5d3f72
Compare
5823a56
to
e704541
Compare
a5d3f72
to
407cd58
Compare
e704541
to
91e489e
Compare
407cd58
to
b6d8453
Compare
91e489e
to
bd76584
Compare
Merge activity
|
b6d8453
to
e374d83
Compare
bd76584
to
55b9187
Compare
## Summary & Motivation Creates a condition which determines if an asset partition has been requested within some lookback period. It does this by keeping track of a set of (essentially) tuples of (AssetSlice, last_requested_timestamp). These tuples are evicted from the list whenever they age out of the lookback window. It uses asset_subsets_with_metadata instead of the more generic extra_state field because asset_subsets_with_metadata get rendered in the UI. this means that users will be able to see exactly when an asset partition was last requested (if it was requested within the given lookback period). ## How I Tested These Changes
Summary & Motivation
Creates a condition which determines if an asset partition has been requested within some lookback period.
It does this by keeping track of a set of (essentially) tuples of (AssetSlice, last_requested_timestamp). These tuples are evicted from the list whenever they age out of the lookback window.
It uses asset_subsets_with_metadata instead of the more generic extra_state field because asset_subsets_with_metadata get rendered in the UI. this means that users will be able to see exactly when an asset partition was last requested (if it was requested within the given lookback period).
How I Tested These Changes