Reflect env variables on global configuration #2371
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent update in the ZenML configuration involves a streamlined approach to accessing the Zen store. The process has been simplified by directly checking the presence of the Zen store and eliminating the previous configuration step. Additionally, an assertion ensures the Zen store's attribute is always set, enhancing the reliability of the system. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
internal
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- src/zenml/config/global_config.py (1 hunks)
Additional comments: 1
src/zenml/config/global_config.py (1)
- 672-672: The modification to the
zen_storeproperty to directly set the default store if_zen_storeisNonesimplifies the logic. However, ensure that removing the_configure_storecall does not omit necessary initializations that might have been performed within_configure_store. The added assertion ensures_zen_storeis notNoneafter attempting to set the default store, which is a good practice for ensuring the property's integrity.
| @@ -669,11 +669,8 @@ def zen_store(self) -> "BaseZenStore": | |||
| Returns: | |||
| The current zen store. | |||
| """ | |||
| if not self.store: | |||
| if self._zen_store is None: | |||
There was a problem hiding this comment.
Can you explain how these changes actually solve the issue? It seems to me like we now always write the default config to disk, doesn't that lose any information that we store in the file?
There was a problem hiding this comment.
does adding a check on self.store after env variables solve the issue? since it will only go to return the get_default_store_config if no env variables where set
There was a problem hiding this comment.
Updated now to do the right thing: use the merged configuration to initialize the store.
src/zenml/config/global_config.py
Outdated
| config = StoreConfiguration( | ||
| **env_store_config, | ||
| ) | ||
| elif self.store: | ||
| config = self.store |
There was a problem hiding this comment.
To add to Michael's comment: it seems to me that in this case either ENV go or File, but not a merger of them, isn't it?
There was a problem hiding this comment.
That's a good point! maybe I understood the ticket differently. In that case, I imagine we can do something like the following, where we see if self.config is set and then update config based on existing env variables. @schustmi @avishniakov
# Update self.config based on environment variables
if self.config:
# Update the store configuration from environment variables
if env_store_config:
if "type" not in env_store_config and "url" in env_store_config:
env_store_config["type"] = BaseZenStore.get_store_type(env_store_config["url"])
# Merge environment variables into self.config for store
for key, value in env_store_config.items():
self.config[key] = value
# Update the secrets store configuration
for key, value in env_secrets_store_config.items():
self.config.secrets_store[key] = value
# Update the backup secrets store configuration
for key, value in env_backup_secrets_store_config.items():
self.config.backup_secrets_store[key] = value
logger.debug("Updated the configuration with environment variables.")
else:
if len(env_store_config):
if "type" not in env_store_config and "url" in env_store_config:
env_store_config["type"] = BaseZenStore.get_store_type(env_store_config["url"])
logger.debug("Using environment variables to configure the default store")
self.config = StoreConfiguration(**env_store_config)
else:
self.config = BaseZenStore.get_default_store_config(
path=os.path.join(self.local_stores_path, DEFAULT_STORE_DIRECTORY_NAME)
)
There was a problem hiding this comment.
Yeah, something like this might do. Can you write some tests to cover this use-case?
There was a problem hiding this comment.
@avishniakov Looking back on this, we can't merge. With the env variables, we need to initialize a new zen store which in this case will be the RestZenStore if we use the already existing one which is SQLZenStore config and we just updated (merge) to initialize the rest one it will fail because rest zen store doesn't the same attributes as sql one and each of these stores has different attributes besides type, URL, and SSL.
There was a problem hiding this comment.
I implemented a custom merge logic that in some cases replaces the config file store completely, in other cases it only overrides some of its attributes.
…lect-env-changes-on-globalconfig
…the persisted one
…lect-env-changes-on-globalconfig
safoinme
deleted the
bugfix/OSSK-36-reflect-env-changes-on-globalconfig
branch
* reflect env variables on global configuration * add check on self.store * Remove unneeded copy_configuration functions * Cleaned up unneded attrs and methods from the global config * Move conversion to env variables to global config * Implemented store environment variables overlay support for global config * Ensure external code uses the actual active store configuration, not the persisted one * Fix docstrings * Fix unit tests --------- Co-authored-by: Stefan Nica <stefan@zenml.io> Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
Describe changes
Fix for the following issue:
Details
This PR implements an environment variable overlay that is applied on top of the current store configuration, regardless of whether it is loaded from the global configuration file or explicitly configured using
set_store. The environment variables are used to either replace the active store configuration or merged on top of them, depending on how complete they are.Downside: if any environment variables are used to configure the zen store, they will also be reflected in the configuration file, thus replacing what was there before. There is no easy way to prevent this, because we rely on the actual running store configuration to be dumped back into the configuration file for persistence purposes (e.g. to persist the API token).
More changes included to clean up the already overloaded global configuration:
copy_configurationmethods that are no longer usedstore_configurationproperty that is the source of truth regarding the actual store configuration, with env variables applied and used it to replace any references to the.storeproperty that is not always as accurate and does not include environment variable overrides.Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes
Summary by CodeRabbit