Skip to content

Commit c65cb74

Browse files
committed
Eliminate strict snapshot id matching (for now)
Summary: We sometimes add fields to classes in the snapshot hierarchy. This causes an issue when we load a historical snapshot, apply the new default values (from added columns) and then recompute the snapshot id. You end up with a different snapshot id then was persisted. This ended up triggering this invariant, which did incorporate this subtley. This diff just disables the invariant for now and adds a test. A more "correct" fix is something more like https://dagster.phacility.com/D2845 which ends up overriding the snapshot ID when fetched from storage. However this is a safer fix in the short term. Test Plan: BK Reviewers: prha, nate Reviewed By: prha Differential Revision: https://dagster.phacility.com/D2843
1 parent e7f22bc commit c65cb74

8 files changed

Lines changed: 62 additions & 3 deletions

File tree

python_modules/dagster/dagster/core/snap/execution_plan_snapshot.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,10 @@ def __init__(self, execution_plan_snapshot, pipeline_index):
2323

2424
self._step_index = {step.key: step for step in self.execution_plan_snapshot.steps}
2525

26-
check.invariant(
27-
execution_plan_snapshot.pipeline_snapshot_id == pipeline_index.pipeline_snapshot_id
28-
)
26+
# https://github.com/dagster-io/dagster/issues/2442
27+
# check.invariant(
28+
# execution_plan_snapshot.pipeline_snapshot_id == pipeline_index.pipeline_snapshot_id
29+
# )
2930

3031
def has_step(self, key):
3132
check.str_param(key, 'key')

python_modules/dagster/dagster_tests/compat_tests/snapshot_0_7_9_shapshot_id_creation_change/sqlite/history/runs.db-wal

Whitespace-only changes.

python_modules/dagster/dagster_tests/compat_tests/snapshot_0_7_9_shapshot_id_creation_change/sqlite/history/runs/e297fa70-49e8-43f8-abfe-1634f02644f6.db-wal

Whitespace-only changes.
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
from dagster.core.instance import DagsterInstance, InstanceRef
2+
from dagster.core.snap import (
3+
ExecutionPlanIndex,
4+
PipelineIndex,
5+
create_execution_plan_snapshot_id,
6+
create_pipeline_snapshot_id,
7+
)
8+
from dagster.utils import file_relative_path
9+
from dagster.utils.test import restore_directory
10+
11+
12+
# a change of schema in the snapshot hierarchy caused hashes to be different
13+
# when snapshots reloaded
14+
def test_0_7_10_snapshot_id_change():
15+
test_dir = file_relative_path(__file__, 'snapshot_0_7_9_shapshot_id_creation_change/sqlite')
16+
with restore_directory(test_dir):
17+
18+
instance = DagsterInstance.from_ref(InstanceRef.from_dir(test_dir))
19+
# run_id = 'e297fa70-49e8-43f8-abfe-1634f02644f6'
20+
21+
old_pipeline_snapshot_id = '88528edde2ed64da3c39cca0da8ba2f7586c1a5d'
22+
old_execution_plan_snapshot_id = '2246f8e5a10d21e15fbfa3773d7b2d0bc1fa9d3d'
23+
24+
pipeline_snapshot = instance.get_pipeline_snapshot(old_pipeline_snapshot_id)
25+
ep_snapshot = instance.get_execution_plan_snapshot(old_execution_plan_snapshot_id)
26+
27+
# It is the pipeline snapshot that changed
28+
# Verify that snapshot ids are not equal
29+
assert create_pipeline_snapshot_id(pipeline_snapshot) != old_pipeline_snapshot_id
30+
31+
assert create_execution_plan_snapshot_id(ep_snapshot) == old_execution_plan_snapshot_id
32+
33+
# This previously failed with a check error
34+
assert ExecutionPlanIndex(ep_snapshot, PipelineIndex(pipeline_snapshot))
35+
36+
37+
# Scripts to create this (run against 0.7.9)
38+
#
39+
# from dagster import pipeline, solid, DagsterInstance, execute_pipeline
40+
# from dagster.core.snap.utils import create_snapshot_id
41+
#
42+
# from dagster.serdes import serialize_pp
43+
#
44+
# @solid
45+
# def noop_solid(_):
46+
# pass
47+
#
48+
# @pipeline
49+
# def noop_pipeline():
50+
# noop_solid()
51+
#
52+
# instance = DagsterInstance.get()
53+
#
54+
# result = execute_pipeline(noop_pipeline, instance=instance)
55+
#
56+
# run_id = result.run_id
57+
58+
# print(serialize_pp(run))

0 commit comments

Comments
 (0)