Skip to content

Commit 5bd48c9

Browse files
OwenKephartDagster Devtools
authored andcommitted
[fix] allow for host cloud uploads of state (#25773)
## Summary & Motivation In host cloud, calls to store state would fail because the state storage commands weren't implemented (as previously all state storage would have happened from user code side of the world) This adds in that functionality and does a small refactor to reduce duplicated code ## Test Plan ## Changelog > The changelog is generated by an agent that examines merged PRs and > summarizes/categorizes user-facing changes. You can optionally replace > this text with a terse description of any user-facing changes in your PR, > which the agent will prioritize. Otherwise, delete this section. Internal-RevId: 4c23bd2d5ff818bbd9996f3b9c00f9f32bc287f8
1 parent 88ee57d commit 5bd48c9

4 files changed

Lines changed: 42 additions & 13 deletions

File tree

python_modules/dagster-cloud/dagster_cloud/storage/defs_state/storage.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,6 @@ def _execute_query(self, query, variables=None, idempotent_mutation=False):
7777
query, variable_values=variables, idempotent_mutation=idempotent_mutation
7878
)
7979

80-
def _get_artifact_key(self, key: str, version: str) -> str:
81-
return f"__state__/{self._sanitize_key(key)}/{version}"
82-
8380
def download_state_to_path(self, key: str, version: str, path: Path) -> None:
8481
download_artifact(
8582
url=self.url,

python_modules/dagster/dagster/_core/storage/defs_state/base.py

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -117,13 +117,18 @@ def set_latest_version(self, key: str, version: str | None) -> None:
117117
def _sanitize_key(self, key: str) -> str:
118118
return re.sub(r"[^A-Za-z0-9._-]", "__", key)
119119

120-
def _get_version_key(self, key: str) -> str:
121-
"""Returns a storage key under which the latest version of a given key's state is stored."""
122-
return f"__version__/{key}"
123-
124-
def _get_state_key(self, key: str, version: str) -> str:
125-
"""Returns a storage key under which a given key's state at a given version is stored."""
126-
return f"__state__/{key}/{version}"
120+
def _get_artifact_key(self, key: str, version: str) -> str:
121+
"""Returns the storage key under which a given defs key's state is stored at a given version.
122+
123+
The ``key`` component is sanitized so the result is safe to use as an S3 key or path
124+
segment. This is the single canonical key layout shared by every remote
125+
``DefsStateStorage``: producers (the agent and ``dg`` CLI) and the consumer (host cloud)
126+
must derive identical keys, so the layout lives here on the base class rather than being
127+
reimplemented per subclass. It is effectively frozen -- changing the prefix or the
128+
sanitization orphans every previously-uploaded state blob. See
129+
``test_artifact_key_layout_is_frozen``.
130+
"""
131+
return f"__state__/{self._sanitize_key(key)}/{version}"
127132

128133
@classmethod
129134
def get(cls) -> Optional["DefsStateStorage"]:

python_modules/dagster/dagster_tests/storage_tests/test_defs_state_storage.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,44 @@
11
import tempfile
2+
from pathlib import Path
23

34
import pytest
45
from dagster._core.instance_for_test import instance_for_test
6+
from dagster._core.storage.defs_state.base import DefsStateStorage
57
from dagster._core.storage.defs_state.blob_storage_state_storage import UPathDefsStateStorage
68
from dagster_shared import check
9+
from dagster_shared.serdes.objects.models.defs_state_info import DefsStateInfo
710
from upath import UPath
811

912
from dagster_tests.storage_tests.utils.defs_state_storage import TestDefsStateStorage
1013

1114

15+
def test_artifact_key_layout_is_frozen():
16+
"""The remote DefsStateStorage key layout is a cross-version contract: the agent and the
17+
``dg`` CLI (producers) and host cloud (consumer) all derive blob keys from the shared
18+
base-class ``_get_artifact_key``. Changing the prefix or the sanitization orphans every
19+
previously-uploaded state blob, so this layout is effectively frozen -- if this test fails
20+
because you changed the layout, you are also signing up to migrate already-stored state.
21+
"""
22+
23+
class _StubDefsStateStorage(DefsStateStorage):
24+
def get_latest_defs_state_info(self) -> DefsStateInfo | None:
25+
raise NotImplementedError
26+
27+
def download_state_to_path(self, key: str, version: str, path: Path) -> None:
28+
raise NotImplementedError
29+
30+
def upload_state_from_path(self, key: str, version: str, path: Path) -> None:
31+
raise NotImplementedError
32+
33+
def set_latest_version(self, key: str, version: str | None) -> None:
34+
raise NotImplementedError
35+
36+
storage = _StubDefsStateStorage()
37+
assert storage._get_artifact_key("MyComponent", "v1") == "__state__/MyComponent/v1" # noqa: SLF001
38+
# The key component is sanitized: anything outside [A-Za-z0-9._-] collapses to "__".
39+
assert storage._get_artifact_key("a/b c", "v1") == "__state__/a__b__c/v1" # noqa: SLF001
40+
41+
1242
class TestDefaultDefsStateStorage(TestDefsStateStorage):
1343
"""Tests the default state storage implementation."""
1444

python_modules/libraries/dagster-dg-cli/dagster_dg_cli/utils/plus/defs_state_storage.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,6 @@ def graphql_client(self) -> IGraphQLClient:
136136
def _execute_query(self, query, variables=None):
137137
return self.graphql_client.execute_arbitrary(query, variables=variables)
138138

139-
def _get_artifact_key(self, key: str, version: str) -> str:
140-
return f"__state__/{self._sanitize_key(key)}/{version}"
141-
142139
def download_state_to_path(self, key: str, version: str, path: Path) -> None:
143140
download_artifact(
144141
url=self.url,

0 commit comments

Comments
 (0)