Separate stack persistence from repo implementation #462

jwwwb · 2022-03-11T11:02:01Z

Describe changes

Creates a BaseStackStore abstract interface to handle the persistence of stacks and stack components, and defers all direct persistence operations in the Repository class to this interface. Introduces new pydantic models wrapping/describing stacks and components to provide an easily serializable object for the interface to the stack store.

Provides 2 implementations of the BaseStackStore: a LocalStackStore that mimics the existing .yaml persistence in the Repository (but in a separate file from the repository config.yaml) and a SQL implementation using SQLModel backend.

This is a purely internal change that should not affect the user at all, but paves the way for the separation of stack storage backend into a service.

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)

…epo-implementation

schustmi

Have a few minor comments, but looks great overall!

src/zenml/enums.py

schustmi · 2022-03-15T09:16:29Z

src/zenml/utils/enum_utils.py

@@ -27,3 +27,8 @@ def __str__(self) -> str:
    def list(cls) -> List[str]:


Not really part of your change, but does it make sense to rename this to names() now that we added a values() equivalent?

schustmi · 2022-03-15T09:28:12Z

src/zenml/stack_stores/models/stack_configuration.py

+    orchestrator: str
+    metadata_store: str
+    artifact_store: str
+    container_registry: Optional[str]


Just a reminder to make sure the step operator doesn't get lost in some merge here

src/zenml/stack_stores/models/stack_component_configuration.py

src/zenml/stack_stores/models/stack_wrapper.py

schustmi · 2022-03-15T09:49:27Z

src/zenml/stack_stores/local_stack_store.py

+
+    @property
+    def root(self) -> Path:
+        """The root directory of this repository."""


Suggested change

"""The root directory of this repository."""

"""The root directory of this stack store."""

src/zenml/stack_stores/local_stack_store.py

schustmi · 2022-03-15T09:58:06Z

src/zenml/stack_stores/sql_stack_store.py

+    StackConfiguration,
+)
+
+# from sqlalchemy import select


Can probably be removed?

schustmi · 2022-03-15T10:01:29Z

src/zenml/stack_stores/sql_stack_store.py

+#  permissions and limitations under the License.
+
+# type: ignore
+# ^ this is needed here in order to keep SQLModel from blowing up mypy.


What errors do you get from mypy when uncommenting this line? We might be able to add some sqlmodel.*ignore in pyproject.toml to ignore some errors for this specific package or disable type checking for it entirely

This is a weird one. it doesn't just error in the sense that I could ignore that, the entire mypy run fails before checking a single file in the project.

±  mypy src Traceback (most recent call last): File "/Users/jwb/Developer/zenml/venvs/zenintel3/bin/mypy", line 8, in <module> sys.exit(console_entry()) File "/Users/jwb/Developer/zenml/venvs/zenintel3/lib/python3.8/site-packages/mypy/__main__.py", line 12, in console_entry main(None, sys.stdout, sys.stderr) File "mypy/main.py", line 96, in main File "mypy/main.py", line 173, in run_build File "mypy/build.py", line 180, in build File "mypy/build.py", line 256, in _build File "mypy/build.py", line 2717, in dispatch File "mypy/build.py", line 3048, in process_graph File "mypy/build.py", line 3165, in process_stale_scc File "mypy/build.py", line 2308, in write_cache File "mypy/build.py", line 1490, in write_cache File "mypy/nodes.py", line 334, in serialize File "mypy/nodes.py", line 3330, in serialize File "mypy/nodes.py", line 3267, in serialize File "mypy/nodes.py", line 941, in serialize File "mypy/types.py", line 1386, in serialize File "mypy/types.py", line 541, in serialize File "mypy/types.py", line 984, in serialize File "mypy/nodes.py", line 2642, in fullname AttributeError: attribute '_fullname' of 'TypeInfo' undefined

Any ideas?

No idea, really weird that it's an actual mypy internal crash. I'll see if the same happens for me or if I can find a solution to it

src/zenml/stack_stores/sql_stack_store.py

stefannica

Great work ! I thoroughly tested it during my ZenProfile adventures, everything works really nice. The legacy repo migration was a particularly nice touch. If there was one thing I'd ask of this PR, it would be that those nasty SQLAlchemy warnings would go away, but not a big deal right now.

schustmi

Great work! Just found two tiny details in one log message and license, other than that good to go

src/zenml/utils/enum_utils.py

schustmi · 2022-03-23T09:49:17Z

src/zenml/repository.py

@@ -128,15 +99,38 @@ def __init__(self, root: Optional[Path] = None):
            )

        self._root = Repository.find_repository(root)
+        logger.debug(" + Initializing %s repo at %s", storage_type, self._root)


Seems to still be there :D

Co-authored-by: Michael Schuster <schustmi@users.noreply.github.com>

bcdurak

Aside from the notes from Michael and Stefan and a really small detail, everything looks really great. This unlocks a lot of potential regarding how we store the stacks.

src/zenml/repository.py

bcdurak · 2022-03-23T10:28:00Z

Perhaps a small question. What do you think about how this will evolve in the future? With the switch between development and deployment, and the collaboration of people within a team, I feel there are a lot of things to unlock here.

jwwwb · 2022-03-23T15:51:34Z

Aside from the notes from Michael and Stefan and a really small detail, everything looks really great. This unlocks a lot of potential regarding how we store the stacks.

Regarding your small detail -- I was aware of it an it's fixed in my follow-up PR, but since stefan is actually removing that option again in his current PR, I thought there's no need to "backport" it to this one. given that it will be gone by tomororw, I'd prefer to avoid giving stefan more merge conflicts :)

jwwwb · 2022-03-23T15:52:51Z

Ooops, did not mean to close this.

jwwwb added 5 commits March 3, 2022 14:29

Rip out stack persistance from Repository implementation

344b5a2

Refactor common code from Local to BaseStackStore

89eeedf

Use correct subclass constructor for component flavors

5ca228f

Add sqlite stackstore backend

bf7a5a9

Make all BaseStackStore interfaces serializable

3043931

jwwwb requested a review from stefannica March 11, 2022 11:02

github-actions bot added internal To filter out internal PRs and issues enhancement New feature or request labels Mar 11, 2022

Merge branch 'develop' into feature/separate-stack-persistance-from-r…

8b8af98

…epo-implementation

jwwwb marked this pull request as ready for review March 11, 2022 12:29

jwwwb requested review from schustmi, htahir1, bcdurak, ayush714 and wjayesh and removed request for stefannica March 11, 2022 12:30

Merge branch 'develop' into feature/separate-stack-persistance-from-r…

0383e3b

…epo-implementation

schustmi requested changes Mar 15, 2022

View reviewed changes

jwwwb added 3 commits March 16, 2022 10:38

Improve API documentation for stack store

98f0165

Remove StackConfiguration in favor of simple dict

3144bb5

Move active_stack back to repository, fix legacy loading

673c89d

jwwwb force-pushed the feature/separate-stack-persistance-from-repo-implementation branch from 26603b0 to 673c89d Compare March 16, 2022 18:59

Let mypy do its thing on sql stack store

85b0edf

jwwwb requested a review from schustmi March 17, 2022 09:26

stefannica approved these changes Mar 21, 2022

View reviewed changes

Add unit tests for public methods of stack store

5aa67e9

jwwwb force-pushed the feature/separate-stack-persistance-from-repo-implementation branch from 7dc125e to 5aa67e9 Compare March 23, 2022 09:28

jwwwb removed request for htahir1, bcdurak and ayush714 March 23, 2022 09:39

jwwwb requested review from schustmi and bcdurak and removed request for wjayesh and schustmi March 23, 2022 09:39

schustmi requested changes Mar 23, 2022

View reviewed changes

jwwwb and others added 2 commits March 23, 2022 11:18

Update src/zenml/utils/enum_utils.py

1ad987b

Co-authored-by: Michael Schuster <schustmi@users.noreply.github.com>

Remove plus sign

3aa5500

bcdurak approved these changes Mar 23, 2022

View reviewed changes

src/zenml/repository.py Show resolved Hide resolved

jwwwb closed this Mar 23, 2022

jwwwb reopened this Mar 23, 2022

jwwwb requested a review from schustmi March 23, 2022 15:53

schustmi approved these changes Mar 23, 2022

View reviewed changes

jwwwb merged commit 8714a8e into develop Mar 23, 2022

jwwwb deleted the feature/separate-stack-persistance-from-repo-implementation branch March 23, 2022 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate stack persistence from repo implementation #462

Separate stack persistence from repo implementation #462

jwwwb commented Mar 11, 2022

schustmi left a comment

schustmi Mar 15, 2022

schustmi Mar 15, 2022

schustmi Mar 15, 2022

schustmi Mar 15, 2022

schustmi Mar 15, 2022

jwwwb Mar 16, 2022

schustmi Mar 16, 2022

stefannica left a comment

schustmi left a comment

schustmi Mar 23, 2022

bcdurak left a comment

bcdurak commented Mar 23, 2022

jwwwb commented Mar 23, 2022

jwwwb commented Mar 23, 2022

		@@ -27,3 +27,8 @@ def __str__(self) -> str:
		def list(cls) -> List[str]:

	"""The root directory of this repository."""
	"""The root directory of this stack store."""

Separate stack persistence from repo implementation #462

Separate stack persistence from repo implementation #462

Conversation

jwwwb commented Mar 11, 2022

Describe changes

Pre-requisites

Types of changes

schustmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefannica left a comment

Choose a reason for hiding this comment

schustmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcdurak left a comment

Choose a reason for hiding this comment

bcdurak commented Mar 23, 2022

jwwwb commented Mar 23, 2022

jwwwb commented Mar 23, 2022