Skip to content
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

Adding mps support to base handler and regression test #3048

Merged
merged 20 commits into from
Apr 9, 2024
Merged

Conversation

udaij12
Copy link
Collaborator

@udaij12 udaij12 commented Mar 27, 2024

Description

Adding MPS as device type for Mac M1. #3022

When Model-config.yaml deviceType: "gpu" or deviceType is not specified then default to GPU:
MPS enabled

When Model-config.yaml deviceType: "cpu":
MPS not enabled

Test cases

Added 4 test cases.
Frontend:

  1. test that number of gpu is greater then 0 when on Mac M1

Backend:

  1. Set deviceType to GPU and test that device == "mps"
  2. Set deviceType to CPU and test that device != "mps"
  3. Dont set deviceType (remains at default) and test that device == "mps"

@@ -149,6 +149,8 @@ def initialize(self, context):
self.device = torch.device(
self.map_location + ":" + str(properties.get("gpu_id"))
)
elif hasattr(self, "model_yaml_config") and "mps" in self.model_yaml_config and self.model_yaml_config["mps"] == "enable":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add mps here

deviceType: cpu # cpu, gpu, neuron

cc @lxning

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of design, having one config for all deviceType probably makes sense?

@lxning
Copy link
Collaborator

lxning commented Mar 27, 2024

  1. CX still can specify deviceType (CPU or GPU) on Mac.
  2. The PR should be able to support getNumber of GPUs on Mac.
  3. The GPU device id is decided by frontend, which is as same as Linux.
  4. Backend handler should be able to auto-detect Mac and apply MPS+deviceId. Essentially, there are no new config parameters in the model-config.yaml.

@udaij12
Copy link
Collaborator Author

udaij12 commented Mar 29, 2024

MPS is auto detected in the base handler and getAvailableGpu() now supports Mac M1 gpu cores.

Copy link
Collaborator

@agunapal agunapal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the User experience for both the cases in the PR/ add this in a README and add a pytest for MPS on/off for a model

@agunapal
Copy link
Collaborator

agunapal commented Apr 1, 2024

cc: @msaroufim

@udaij12 udaij12 marked this pull request as ready for review April 2, 2024 17:14
ts/torch_handler/base_handler.py Show resolved Hide resolved
ts/torch_handler/base_handler.py Outdated Show resolved Hide resolved
ts/torch_handler/base_handler.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@lxning lxning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add test cases?

  1. define deviceType in model-config.yaml on Mac
  2. cpu
  3. gpu
  • deviceId
  1. define deviceType in model-config.yaml on gpu host

@lxning
Copy link
Collaborator

lxning commented Apr 8, 2024

@udaij12 in general, LGTM. I just left a minor comments #3048 (comment). Thanks.

@agunapal
Copy link
Collaborator

agunapal commented Apr 8, 2024

@udaij12 Can you please add a document called apple_silicon_support.md in docs folder where you mention these details. also please include what is currently working

Copy link
Collaborator

@agunapal agunapal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@msaroufim msaroufim self-requested a review April 8, 2024 22:51
@udaij12 udaij12 enabled auto-merge April 8, 2024 23:39
@udaij12 udaij12 added this pull request to the merge queue Apr 9, 2024
Merged via the queue into master with commit 89c5389 Apr 9, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants