New core architecture #305
Conversation
schustmi
requested review from
strickvl,
stefannica,
htahir1,
jwwwb,
bcdurak and
AlexejPenner
|
Looking great so far. So many big and significant changes. Really looking forward to the final version. |
| # set back the old value as we don't want to permanently store | ||
| # the environment variable value here | ||
| super().__setattr__(key, value) | ||
| return return_value |
There was a problem hiding this comment.
Ok thats interesting how you did it here. I hope there are tests for this in the PR below but its a nice workaround for now!
src/zenml/config/global_config.py
Outdated
| options from a legacy config file or returns an empty dictionary. | ||
| """ | ||
| legacy_config_file = os.path.join( | ||
| GlobalConfig.config_directory(), ".zenglobal.json" |
There was a problem hiding this comment.
".zenglobal.json" -> probably best as a constant somewhere
|
@schustmi I guess it's a breaking change so people will realise, but I think people will have to delete their (When I was trying some CLI commands, it said that there was no orchestrator registered etc, which makes sense but might be unexpected for anyone upgrading.) |
There was a problem hiding this comment.
No real comments of substance from me (aside from some tiny nits here and there). I appreciated the ZenShare last week which gave me some context for reading around in the code.
I love the extensive tests, too, and yeah, in general seems like this PR will set us up really well for the future.
| recursively searching in the parent directories of the current | ||
| working directory. | ||
|
|
||
| Raises: |
There was a problem hiding this comment.
Mainly for me to understand this: is it normal for a method's docstring to state that it could raise an error if that error is raised in a separate function/method? In this case it's Repository.find_repository() that's going to raise the error if it needs raising.
There was a problem hiding this comment.
My personal option, but I feel especially for user-facing API it's important to mention as many potential exceptions (even coming from calls to other functions) as possible so they now what to catch in their code.
| """Creates a local stack with components with the given names. If the | ||
| names are not given, a random string is used instead.""" | ||
|
|
||
| def _random_name(): |
There was a problem hiding this comment.
Elsewhere in the repository we use hypothesis for doing things like this. It's potentially worth the extra effort to integrate that in since it might raise some weirdnesses that we'd otherwise not expect.
There was a problem hiding this comment.
How would you use hypothesis in this case when it is not an actual test function but a helper function?
There was a problem hiding this comment.
You'd get rid of the helper function and instead use the hypothesis @given decorator (I think) on the test and pass whatever random string(s) were generated by hypothesis. You'd need to make sure to pass in a min_size=1 along with the text() strategy.
There was a problem hiding this comment.
I tried but it doesn't work in combination with our clean repo fixture
There was a problem hiding this comment.
E
E Function-scoped fixtures are not reset between examples generated by
E `@given(...)`, which is often surprising and can cause subtle test bugs.
E
E If you were expecting the fixture to run separately for each generated example,
E then unfortunately you will need to find a different way to achieve your goal
E (e.g. using a similar context manager instead of a fixture).```
There was a problem hiding this comment.
Ah yes, I'd forgotten about that. Do you hit a test strategy scope error?
There was a problem hiding this comment.
The error was called FailedHealthCheck
There was a problem hiding this comment.
Not sure if this applies in this case, but you can change the scope with @pytest.fixture(scope="module")
There was a problem hiding this comment.
Unfortunately not, this fixture creates a clean repo for one specific test so has to stay with function scope. Anyway I think the random strings in this case don't really matter as it's just for the names, but I agree we should definitely use this in cases where the strings are more relevant!
|
@alex-zenml Maybe we could implement some logic for migrating this specific release, but I feel like the effort isn't worth it. We've asked users to delete and recreate their .zen folder before and I feel like that's the way to go with this release as well |
|
@schustmi agreed. I guess I'm only wondering about the people with several hundred runs in their belt. As long as we're clear that this is a breaking change in the release notes it's probably fine. |
|
Such an amazing effort! Thanks to the ZenShare, it was relatively easy to comprehend what was going on with the PR and it looks solid almost from all angles. |
Pre-requisites
Please ensure you have done the following:
Types of changes
Describe changes
This PR implements fundamental changes to the core architecture to solve some of the issues we previously had and hopefully provides a more extensible design to support quicker implementations of different stack components/integrations.
This PR modifies lots of files, but the main changes are the following:
Repository/Stack/StackComponentarchitecture (seesrc/zenml/repository.pyandsrc/zenml/stack/*.py)GCPArtifactStore) to work with these new classessrc/zenml/cli/stack_components.py)Most of the remaining changes are: