added GPU environment support#11
Conversation
jaipreet-s
left a comment
jaipreet-s
left a comment
There was a problem hiding this comment.
It'd be great if we can add an example on how to use this and the compute_type parameter in the README's Usage section as well https://github.com/aws-samples/sagemaker-studio-image-build-cli#usage
| self.domain_id, self.user_profile_name = self._get_studio_metadata() | ||
| self.repo_name = None | ||
| self.compute_type = compute_type or "BUILD_GENERAL1_SMALL" | ||
| self.environment = environment or "LINUX_CONTAINER" |
There was a problem hiding this comment.
This default is being specified in multiple places - in the argument parsing and here. From what I can tell, the environment param should always be non-null when we get to this point, so we can remove this or check.
| if self.environment=="LINUX_GPU_CONTAINER" and self.compute_type=="BUILD_GENERAL1_SMALL": | ||
| self.compute_type="BUILD_GENERAL1_LARGE" |
There was a problem hiding this comment.
Question: Why is this required?
There was a problem hiding this comment.
BUILD_GENERAL1_LARGE will automatically switch to a GPU instance if LINUX_GPU_CONTAINER is set. Small or medium will not.
There was a problem hiding this comment.
Thanks. In that case, I recommend we make this explicit - Fail if the user is requested GPU and Small or Medium, because these are incompatible configuration options
|
Sorry for the long delay. I've updated the changes so |
2 participants
Issue #, if available:
Issue #10
Description of changes:
Adds
--environmentoption to setLINUX_GPU_CONTAINERwhen build involves compiling Cuda operations.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.