-
Notifications
You must be signed in to change notification settings - Fork 11
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
Ray AWS Cluster Config Generator #47
Conversation
Job PR-47-96d5217 is done. |
def _update_config( | ||
self, | ||
instance_type: Optional[str] = None, | ||
instance_count: Optional[str] = None, |
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.
Why not set it to 1 as a default and change type to int
in the API?
instance_count: int = 1
?
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.
It should be int, that's a mistake. Though if we specify a default value for instance_count, we will always update the config to have instance_count = 1 even when user left it blank. I'll stick with default = None, meaning user doesn't want to update it
src/autogluon/cloud/cluster/ray_aws_cluster_config_generator.py
Outdated
Show resolved
Hide resolved
src/autogluon/cloud/cluster/ray_aws_cluster_config_generator.py
Outdated
Show resolved
Hide resolved
def _set_provider(self): | ||
"""Set provider to be default ones if user didn't provide any""" | ||
default_config = self._default_config | ||
provider: Dict[str, Any] = self.config.get(PROVIDER, None) | ||
if provider is None: | ||
provider = default_config[PROVIDER] | ||
self.config.update({PROVIDER: provider}) |
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.
Can we templatize repeating methods: I see lots of repeating code which can be reuse between the _update_xxx
methods.
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.
It is hard to templatize these methods because they are essentially trying to update keys and values lying in different layers of the yaml file. The method looks similar, but because the structure of the yaml file differs for each key, it's actually specific
src/autogluon/cloud/cluster/ray_aws_cluster_config_generator.py
Outdated
Show resolved
Hide resolved
Job PR-47-a74b280 is done. |
Issue #, if available:
Description of changes:
RayAWSClusterConfigGenerator
. It has a default config and accepts user config override. Also allow users to update the config with frequent used parameters that are available infit
call, i.e.instance_type
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.