-
Notifications
You must be signed in to change notification settings - Fork 800
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
Fix #73 - Calcs now correctly the TFLOPS for MacBook Pro M1 Max #74
base: main
Are you sure you want to change the base?
Conversation
Awesome work! Thanks so much for the PR. Could you also update the tests in |
exo/topology/device_capabilities.py
Outdated
|
||
# Assuming static values for other attributes for demonstration | ||
return DeviceCapabilities(model=model_id, chip=chip_id, memory=memory, flops=CHIP_FLOPS.get(chip_id, DeviceFlops(fp32=0, fp16=0, int8=0))) | ||
hw_info = subprocess.check_output(['system_profiler', 'SPHardwareDataType']).decode('utf-8') |
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.
I just took another look and there's a -json
flag that might make parsing a bit cleaner: system_profiler SPHardwareDataType -json
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.
Indeed, removed "re" and used json processing instead:
system_profiler SPHardwareDataType -json
{
"SPHardwareDataType" : [
{
"_name" : "hardware_overview",
"activation_lock_status" : "activation_lock_disabled",
"boot_rom_version" : "10151.101.3",
"chip_type" : "Unknown",
"machine_model" : "MacBookPro18,2",
"machine_name" : "MacBook Pro",
"model_number" : "XXXXXXX",
"number_processors" : "proc 10:8:2",
"os_loader_version" : "10151.101.3",
"physical_memory" : "64 GB",
"platform_UUID" : "AXXXXXXX",
"provisioning_UDID" : "AXXXXXXX",
"serial_number" : "XXXXXXX"
}
]
}
exo/topology/device_capabilities.py
Outdated
|
||
# Extract relevant information | ||
model_id = re.search(r"Model Name: (.*)", hw_info).group(1).strip() | ||
chip_id = "Unknown Chip" |
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.
Shouldn't we start by trying to parse this from the system_profiler
response?
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.
Done, see latest commit
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.
Really? It still says chip_id = "Unknown Chip"
. I don't see where it sets the chip_id
based on system_profiler
response.
exo/topology/device_capabilities.py
Outdated
@@ -77,24 +76,60 @@ def device_capabilities() -> DeviceCapabilities: | |||
else: | |||
return DeviceCapabilities(model=f"Unknown Device", chip=f"Unknown Chip", memory=psutil.virtual_memory().total // 2**20, flops=DeviceFlops(fp32=0, fp16=0, int8=0)) | |||
|
|||
|
|||
import subprocess | |||
import re |
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.
these imports at top
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.
Replaced regex module "re" with "json"
Looks like you might've accidentally committed your |
exo/topology/device_capabilities.py
Outdated
# Try to identify the chip using sysctl | ||
try: | ||
sysctl_output = subprocess.check_output(['sysctl', '-n', 'machdep.cpu.brand_string']).decode('utf-8').strip() | ||
if "Apple M1" in sysctl_output: |
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 generalise to "Apple M{N}" where N can be 1,2,3,....
Please also remove |
- Refactor tests to use JSON-based system_profiler output - Add mocked test for MacBook Pro with M3 Max chip - Improve robustness of chip detection in mac_device_capabilities function - Update assertions to match new JSON parsing logic
Needs some more work because now I'm getting again 0 TFLOPS... hold on |
No description provided.