Update stack and stack components via the CLI #497

strickvl · 2022-04-04T13:35:01Z

I added functionality to allow users to update stacks that already exist. This shows the basic workflow:

zenml orchestrator register local_orchestrator2 -t local
zenml stack update default -o local_orchestrator2
zenml stack describe default
zenml container-registry register local_registry --type=default --uri=localhost:5000
zenml container-registry update local --uri='somethingelse.com'
zenml container-registry rename local local2
zenml container-registry describe local2
zenml stack rename default new_default
zenml stack update new_default -c local2
zenml stack describe new_default
zenml stack remove-component -c

More details are in the CLI docs (updated as part of this PR). Users can add new stack components to a pre-existing stack, or they can modify already-present stack components. They can also rename their stack and individual stack components.

Remaining tasks:

Add REST stack store functionality
Remove / refactor out helper function written for test_stack_store which is causing wonky functionality
Update CLI docs to add component update and rename functionality

Pre-requisites

Please ensure you have done the following:

I have read the CONTRIBUTING.md document.
If my change requires a change to docs, I have updated the documentation accordingly.
If I have added an integration, I have updated the integrations table.
I have added tests to cover my changes.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Other (add details above)

schustmi

Nice work! Only left a few small comments

src/zenml/repository.py

tests/unit/test_repository.py

schustmi · 2022-04-05T15:26:09Z

src/zenml/stack_stores/base_stack_store.py

+        def __check_component(
+            component: StackComponentWrapper,
+        ) -> Tuple[StackComponentType, str]:
+            try:
+                _ = self.get_stack_component(
+                    component_type=component.type, name=component.name
+                )
+            except KeyError:
+                self.register_stack_component(component)
+            return component.type, component.name
+
+        stack_configuration = {
+            typ: name for typ, name in map(__check_component, stack.components)
+        }


Maybe we can extract this common code from both registering and updating a stack into some separate function that both of them can call?

src/zenml/stack_stores/sql_stack_store.py

src/zenml/cli/stack.py

jwwwb

Oops, I wrote a few comments but forgot to submit the review. Looks good, just have a few small ideas that could be changed.

jwwwb · 2022-04-06T05:30:43Z

src/zenml/cli/stack.py

+    try:
+        current_stack = repo.get_stack(stack_name)
+    except KeyError as e:
+        console.print(


Shouldn't this also return from the function? It seems like all the stack_components key access below will fail

I'll just raise a cli_utils.error instead of printing. Good catch.

jwwwb · 2022-04-06T14:07:19Z

src/zenml/stack_stores/base_stack_store.py

+    @abstractmethod
+    def update_stack_component(
+        self,
+        current_component: StackComponentWrapper,


Since this is all unimplemented, Maybe consider leaving it out? But if it does stay in, I think the function signature should not take 2 StackComponentWrappers, but rather a type and name of the component to update, and a wrapper to update it with (same signature as the create stack component)

jwwwb · 2022-04-06T14:10:51Z

tests/unit/test_repository.py

    )


+def test_updating_a_new_stack_with_already_registered_components(clean_repo):


I would suggest maybe testing in the stack store tests, because those are parameterized to test every implementation, whereas this will only test the yaml version I believe.

Good idea. I've moved it over.

…date-stack-components

schustmi · 2022-04-25T15:18:36Z

src/zenml/cli/stack.py

+        stack_components = current_stack.components
+
+        if container_registry_flag:
+            stack_components = {


I think this could also be solved by a stack_components.pop(StackComponentType.CONTAINER_REGISTRY, None) as well to make it a little shorter. That will also remove a key-value pair from a dictionary in case a value for that key exists.

schustmi · 2022-04-25T15:20:20Z

src/zenml/cli/stack.py

+            )
+        stack_components = current_stack.components
+
+        registered_stacks = [stack.name for stack in repo.stacks]


Suggested change

registered_stacks = [stack.name for stack in repo.stacks]

registered_stacks = {stack.name for stack in repo.stacks}

I would suggest using a set here as it makes conceptually more sense for a lookup

schustmi · 2022-04-25T15:23:35Z

src/zenml/cli/stack_components.py

+    command_group.command(
+        "update",
+        context_settings=context_settings,
+        help=f"Update a new {singular_display_name}.",


Suggested change

help=f"Update a new {singular_display_name}.",

help=f"Update a registered {singular_display_name}.",

schustmi · 2022-04-25T15:29:03Z

src/zenml/cli/stack_components.py

+            registered_components = [
+                component.name
+                for component in repo.get_stack_components(component_type)
+            ]


Suggested change

registered_components = [

component.name

for component in repo.get_stack_components(component_type)

]

registered_components = {

component.name

for component in repo.get_stack_components(component_type)

}

schustmi · 2022-04-25T15:30:47Z

src/zenml/cli/stack_components.py

+            for prop in required_properties:
+                if prop not in parsed_args:
+                    parsed_args[prop] = getattr(current_component, prop)
+            updated_component = component_class(name=name, **parsed_args)


If the component.update worked in the rename method, it might be worth trying it here as well like this:

updated_component = current_component.copy(update=parsed_args)

schustmi · 2022-04-25T15:41:48Z

src/zenml/stack_stores/local_stack_store.py

+            self._delete_stack_component(component_type, name)
+
+        # add the component to the stack store dict and write it to disk
+        components[component.name] = component.flavor


I know that you're already checking for the case that a component exists in the CLI methods (and I definitely think it should be there with a CLI error message), however I think we should have it here as well. Users (or us) might still call this function directly in which case we would just overwrite the existing one without noticing. Maybe a StackComponentExistsError?

schustmi · 2022-04-25T15:43:52Z

src/zenml/stack_stores/base_stack_store.py

+        try:
+            self.get_stack(name)
+        except KeyError:
+            raise DoesNotExistException(


I would keep this as a KeyError (and get rid of the DoesNotExistException entirely) as we use that in all other cases as well.

schustmi · 2022-04-25T15:46:10Z

src/zenml/stack_stores/base_stack_store.py

+            except KeyError:
+                self.register_stack_component(component)
+            return component.type, component.name
+


Same as the stack component, I think we should still have a check here if something with the new name already exists. Probably a StackExistsError?

schustmi · 2022-04-25T15:51:56Z

src/zenml/stack_stores/sql_stack_store.py

+            component_type,
+            component.name,
+        )
+        # tc = session.exec(


Can probably be removed? Similar to the local store, I think we should handle a component with that name already existing

schustmi · 2022-04-25T15:53:44Z

src/zenml/stack_stores/sql_stack_store.py

-            session.add(stack)
+            stack = session.exec(
+                select(ZenStack).where(ZenStack.name == name)
+            ).first()


Similar to the local store, I think we should handle a stack with that name already existing

Yes I think this is now handled.

…date-stack-components

strickvl · 2022-04-25T19:25:12Z

@schustmi Addressed your comments in these latest commits.

schustmi

This PR was great a few iterations ago already and it got even better!

stefannica

Awesome work ! Thanks so much for considering my suggestions.

stefannica · 2022-04-26T07:33:12Z

src/zenml/stack_stores/local_stack_store.py

+                f"found with this name."
+            )
+        elif name != component.name and component.name in components:
+            breakpoint()


You forgot a breakpoint here :)

…date-stack-components

strickvl added 8 commits March 31, 2022 15:31

add update_stack method (partially)

bde7ae0

update docs for stack update

cba733c

add secrets manager to update stack command params

6ca0ea3

add full update_stack logic

8d75bfa

add new update method to base_stack_store

9c7c0b4

update stack update methods

9fe4f48

add update functionality

cefedf5

add test to check update works

7c7ae87

strickvl added enhancement New feature or request internal To filter out internal PRs and issues labels Apr 4, 2022

strickvl requested review from jwwwb and schustmi April 4, 2022 13:35

stefannica mentioned this pull request Apr 5, 2022

Create base deployer and refactor MLflow deployer implementation #489

Merged

8 tasks

schustmi requested changes Apr 5, 2022

View reviewed changes

jwwwb suggested changes Apr 6, 2022

View reviewed changes

strickvl added 9 commits April 11, 2022 14:50

merge origin/develop

43b5e65

add rich functionality

5b2249e

Merge remote-tracking branch 'origin/develop' into feature/ENG-480-up…

7f6ac49

…date-stack-components

update progress

834e1a0

add update command for individual stack components

6148c85

update test name

8989e60

Merge remote-tracking branch 'origin/develop' into feature/ENG-480-up…

2fb1352

…date-stack-components

remove unnecessary comment

833533d

fix PR comments and failing test

c09b319

strickvl changed the title ~~Update stack components via the CLI~~ Update stack and stack components via the CLI Apr 12, 2022

strickvl added 5 commits April 12, 2022 15:54

add test from repository to stack_stores (lower level)

d4282e5

add put method to support rest_stack_store update

c28f782

fix broken rest stack store test

b5daa76

add stack store test

91430e2

Merge remote-tracking branch 'origin/develop' into feature/ENG-480-up…

cc9097f

…date-stack-components

strickvl added 10 commits April 25, 2022 10:39

add test to cover pre-existing stack rename

b3d103e

add test to cover pre-existing stack-component rename

783de03

update tests

87aa166

fix SQL bug introduced last week

5d92482

fix update_stack put URL

97926ee

fix update_stack_components SQL bug

c89ef77

fix broken tests

87b3cfe

fix broken repo test

2105e46

add docstrings

2e199e0

Merge remote-tracking branch 'origin/develop' into feature/ENG-480-up…

75e1448

…date-stack-components

strickvl requested review from schustmi and stefannica and removed request for htahir1 April 25, 2022 14:33

schustmi requested changes Apr 25, 2022

View reviewed changes

strickvl added 5 commits April 25, 2022 20:17

refactor using pop

2fc369d

update with PR fixes

3c84271

update with PR fixes

d57f2e4

update with PR fixes

02a7856

Merge remote-tracking branch 'origin/develop' into feature/ENG-480-up…

e193f99

…date-stack-components

strickvl requested a review from schustmi April 25, 2022 19:25

schustmi approved these changes Apr 26, 2022

View reviewed changes

Fix SQL store creation through CLI (profile) [ci skip]

4268dd4

stefannica approved these changes Apr 26, 2022

View reviewed changes

schustmi and others added 4 commits April 26, 2022 10:39

Merge remote-tracking branch 'origin/develop' into feature/ENG-480-up…

6e3196c

…date-stack-components

Formatting

a6f583f

Replace remaining stack store occurences

45e52a0

remove leftover breakpoint

6e32dbf

strickvl merged commit 54181d0 into develop Apr 26, 2022

strickvl deleted the feature/ENG-480-update-stack-components branch April 26, 2022 11:41

		)


		def test_updating_a_new_stack_with_already_registered_components(clean_repo):

	registered_stacks = [stack.name for stack in repo.stacks]
	registered_stacks = {stack.name for stack in repo.stacks}

	help=f"Update a new {singular_display_name}.",
	help=f"Update a registered {singular_display_name}.",

Update stack and stack components via the CLI #497

Update stack and stack components via the CLI #497

Uh oh!

Conversation

strickvl commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remaining tasks:

Pre-requisites

Types of changes

Uh oh!

schustmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jwwwb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strickvl commented Apr 25, 2022

Uh oh!

schustmi left a comment

Choose a reason for hiding this comment

Uh oh!

stefannica left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

strickvl commented Apr 4, 2022 •

edited

Loading