-
Notifications
You must be signed in to change notification settings - Fork 34
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
Multiple backends refactor #396
Conversation
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.
looking good so far!
if storage_options is not None and "composite_config" in storage_options: | ||
composite_config = storage_options.get("composite_config", []) | ||
repositories = [] | ||
for config in composite_config: | ||
self.persistence, self.root_dir, self.is_auto_git_enabled = self._load_config( | ||
config["persistence"], config["root_dir"], is_auto_git_enabled | ||
) | ||
repositories.append(self._get_repository()) | ||
|
||
self.repositories = repositories | ||
else: | ||
self.persistence, self.root_dir, self.is_auto_git_enabled = self._load_config( | ||
persistence, root_dir, is_auto_git_enabled | ||
) | ||
self.repository = self._get_repository() |
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.
🔥 awesome, this is exactly what I wanted to clean up
rubicon_ml/client/rubicon.py
Outdated
Config( | ||
persistence=config["persistence"], | ||
root_dir=config["root_dir"], | ||
auto_git_enabled=auto_git_enabled, | ||
) |
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.
this also needs **storage_options
I think we should ultimately just make users pass a list of Config
s if they want to use multiple backends - what do you think? we don't need to do it in this PR
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.
Good catch. Yes I was hoping to split this into two PRs. This one is mostly backwards compatible, and that change would require an API change.
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.
Also the fact that this didn't cause any unit tests to fail indicates a gap - let me address that before we merge here.
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.
Yes I was hoping to split this into two PRs. This one is mostly backwards compatible, and that change would require an API change.
perfect, sounds good!
Also the fact that this didn't cause any unit tests to fail indicates a gap - let me address that before we merge here.
thanks!
…alone/rubicon-ml into multiple_backends_refactor
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.
awesome, thanks!
What
Config
options.How to Test