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
Invalidating artifact/metadata store if there is a change in one of them #719
Conversation
4 similar comments
1 similar comment
…om/zenml-io/zenml into bugfix/ENG-796-invalidating-stores
…-io/zenml into bugfix/ENG-796-invalidating-stores
@AlexejPenner @fa9r I have fixed the issue regarding the UUIDs and the SQL ZenStore, added new tests, and updated the docs accordingly. |
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.
Great changes, love the new argument name and descriptions, very clear now!
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.
LGTM - just out of curiosity, how does this interact with existing stacks? Is this a breaking change?
@@ -500,7 +499,7 @@ def requirements( | |||
] | |||
return set.union(*requirements) if requirements else set() | |||
|
|||
def validate(self) -> None: | |||
def validate(self, decouple_stores: bool = False) -> None: |
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.
Functionality-wise your implementation looks great, the only thing I'm a little concerned about is that the functionality of resetting the store associations doesn't really belong in the validation.
I'd see it more as a method on the BaseZenStore
, e.g.
class BaseZenStore:
def decouple_stores(artifact_store_uuid, metadata_store_uuid):
...
class Repository:
def register_stack(stack, decouple):
if decouple:
self.zen_store.decouple_stores(...)
self.zen_store.register_stack(...)
I know this is quite late in the review and I just randomly took a look, feel free to ignore it in this PR but I think it makes sense to split it up longterm.
Describe changes
I implemented a mechanism that puts a stop to any operation if the corresponding artifact store/metadata store pair is not properly associated.
It works as follows:
uuid
s of the artifact- and metadata stores are saved in pairs in this table.validate
method when you run a pipeline, register/update stacks, etc. This method now also checks whether the stack in question has the correct entry in this table for its artifact- metadata store pair.-r
when creating/updating stack over the CLI, which will reset the previous associations of these components and establish a new one.TODO:
Pre-requisites
Please ensure you have done the following:
Types of changes