[2/s] contextually save DagsterInstance for use in component loading #31358
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
f842374 to
7d52c8c
Compare
f24f7f4 to
1e4d52f
Compare
222633b to
67d5aeb
Compare
1e4d52f to
2cc1189
Compare
2cc1189 to
8349f1b
Compare
d0b1ae9 to
63e9527
Compare
63e9527 to
dfde473
Compare
8349f1b to
90eb27a
Compare
dfde473 to
9cd14f2
Compare
9cd14f2 to
b54a941
Compare
9ada543 to
a5da656
Compare
af35cdb to
3352ae0
Compare
There was a problem hiding this comment.
do we have another StateStore thats not the instance ? If not could we defer this until we have one?
Can we leverage the DagsterInstance.__enter__ for managing a current active instance ctx var? Seeing all these new callsites for StateStore ctx management spooks me.
3352ae0 to
ac8f7bc
Compare
a5da656 to
b7aa2ed
Compare
|
@alangenfeld I've updated this pr to make Regarding the |
which callsites are you targeting that do not enter the instance context manager after instantiating it via ref? I see a few misses but the majority of places seem to enter the context directly or via exit stack |
ac8f7bc to
e995e00
Compare
b7aa2ed to
d7fb077
Compare
| def load_state_to_path(self, key: str, version: str, path: Path) -> None: | ||
| """Loads the state file for the given defs key and version into the given file path. | ||
|
|
||
| Args: | ||
| key (str): The key of the state to retrieve. | ||
| version (str): The version of the state to retrieve. | ||
| path (Path): The path to write the state to. | ||
|
|
||
| Returns: | ||
| bool: True if the state was loaded, False otherwise. | ||
| """ |
There was a problem hiding this comment.
The docstring for load_state_to_path indicates it returns a boolean value (Returns: bool: True if the state was loaded, False otherwise), but the implementation raises an exception on failure and doesn't return anything on success. This creates a mismatch between the documented behavior and actual implementation. Either update the docstring to remove the return type or modify the implementation to return True after successfully writing to the path.
| def load_state_to_path(self, key: str, version: str, path: Path) -> None: | |
| """Loads the state file for the given defs key and version into the given file path. | |
| Args: | |
| key (str): The key of the state to retrieve. | |
| version (str): The version of the state to retrieve. | |
| path (Path): The path to write the state to. | |
| Returns: | |
| bool: True if the state was loaded, False otherwise. | |
| """ | |
| def load_state_to_path(self, key: str, version: str, path: Path) -> None: | |
| """Loads the state file for the given defs key and version into the given file path. | |
| Args: | |
| key (str): The key of the state to retrieve. | |
| version (str): The version of the state to retrieve. | |
| path (Path): The path to write the state to. | |
| """ | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
haven't totally digested this PR yet but +1 to alex's last comment - the only places I see not using from_ref() outside of a contextmanager with are either old celery/dagstermill code or places that are callsites for creating an instance, which should then also be using a |
| StateStorage.set_current( | ||
| instance.state_storage | ||
| if isinstance(instance, DagsterInstance) | ||
| else DagsterInstance.from_ref(instance).state_storage |
There was a problem hiding this comment.
yeah this does not look quite right to me in terms of lifecycle management. Is it a big lift to make this method require an instance instead of an instance_ref? Or use an ExitStack here will creating the execution plan?
| @@ -538,10 +538,10 @@ def get_execution_plan( | |||
| asset_selection=remote_job.asset_selection, | |||
| asset_check_selection=remote_job.asset_check_selection, | |||
| ), | |||
| instance=instance or DagsterInstance.get(), | |||
There was a problem hiding this comment.
this also does not seem quite right
e995e00 to
f7e4bb7
Compare
d7fb077 to
324ebb6
Compare
Summay
Adds a base
StateStoreclass which defines an interface for saving and loading defs state - theDagsterInstanceimplements this interface.StateStorestores a global, current instance of the state, which is set in various Dagster load paths to the instance, so that it is contextually available to user code.Test Plan
Unit tests that the state store is populated in various user code contexts.