AssetCheckSummaryRecord #21940
AssetCheckSummaryRecord #21940
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
dpeng817
force-pushed
the
dpeng817/asset_state_api
branch
from
9ee530c
to
5347d1a
Compare
dpeng817
force-pushed
the
dpeng817/asset_state_api
branch
from
5347d1a
to
5fef025
Compare
There was a problem hiding this comment.
Some more cosmetic nits... I think the storage fixtures are confusing.
Bigger issue is to get a better handle on the API signature. I think @schrockn voiced some reservations about the "state" terminology. We can do the conservative thing and maybe do get_latest_asset_check_info or something?
| @@ -164,6 +164,7 @@ | |||
| from dagster._core.storage.daemon_cursor import DaemonCursorStorage | |||
| from dagster._core.storage.event_log import EventLogStorage | |||
| from dagster._core.storage.event_log.base import ( | |||
| AssetCheckState, | |||
There was a problem hiding this comment.
will defer to @schrockn on AssetCheckState vs LatestAssetCheckInfo.
| @@ -452,6 +454,13 @@ def event_log_storage(self, request): | |||
| def instance(self, request) -> Optional[DagsterInstance]: | |||
| return None | |||
|
|
|||
| @pytest.fixture | |||
| def end_instance(self, instance): | |||
There was a problem hiding this comment.
this is confusing to me... Why are they different?
There was a problem hiding this comment.
See the internal PR; but the essential thing is that in the graphql event log storage tests, the "instance" fixture is expected to be the host instance, rather than the user code instance. So we need a separate instance for the "user code instance" for the cases where they are different.
There was a problem hiding this comment.
I think the reason this is confusing is that we are presumably testing the event log storage implementation.
But we don't make use of the fixtured storage at all in the test. We're effectively testing the implementation of DagsterInstance rather than any particular implementation of the EventLogStorage.
Instead, I would expect to see the following:
def test_get_asset_check_state(self, storage: EventLogStorage):
...
events = _synthesize_events(...)
for event in events:
storage.store_event(event)
latest_check_info = storage.get_latest_check_info(...)
# assert things about the latest check info here
This way, we don't have to touch the instance at all, don't have to be aware that there is a storage class on the instance or anything.
There was a problem hiding this comment.
The nice thing about this is that we're really just testing the relationship between store_event and get_asset_check_state / get_latest_check_info / whatever we decide to call this.
|
Idea: We could call these "summary records". It is a rollup of information about a particular key at a given point in time. One use case I am imagining is generating a log of these over time, so you could understand why the system made decisions when it did. "State record" could also work. But in general I like the notion of also thinking of these as a varietal of record. |
dpeng817
force-pushed
the
dpeng817/asset_state_api
branch
from
5fef025
to
ba974e4
Compare
dpeng817
force-pushed
the
dpeng817/asset_state_api
branch
from
ba974e4
to
86ac348
Compare
Adds an "AssetCheckState" API; which functions similarly to an AssetRecord. The core conceit is to be able to query just based on asset check key a set of information about the "state" of the asset check (most recent run, for now), without having any references to the event log. For now, it's actually powered by the event log, however. How I tested this - Added a new semi-end-to-end test to our event log storage tests. This required me setting up an instance hooked up to an event log storage; which I think is nice to have anyway. - There's one failing test here that's confusing me; test_get_event_records_sqlite. I changed it to use the instance passed in by fixture, which should in theory not change the results? But it seems like it has. Probably missing something here. There will be a follow up cloud pr.
Adds an "AssetCheckState" API; which functions similarly to an AssetRecord. The core conceit is to be able to query just based on asset check key a set of information about the "state" of the asset check (most recent run, for now), without having any references to the event log. For now, it's actually powered by the event log, however. How I tested this - Added a new semi-end-to-end test to our event log storage tests. This required me setting up an instance hooked up to an event log storage; which I think is nice to have anyway. - There's one failing test here that's confusing me; test_get_event_records_sqlite. I changed it to use the instance passed in by fixture, which should in theory not change the results? But it seems like it has. Probably missing something here. There will be a follow up cloud pr.
Adds an "AssetCheckState" API; which functions similarly to an AssetRecord. The core conceit is to be able to query just based on asset check key a set of information about the "state" of the asset check (most recent run, for now), without having any references to the event log.
For now, it's actually powered by the event log, however.
How I tested this
There will be a follow up cloud pr.