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

Determine total memory without psutil #12787

Merged
merged 15 commits into from
Sep 11, 2024
Merged

Determine total memory without psutil #12787

merged 15 commits into from
Sep 11, 2024

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Aug 13, 2024

Fixes #12785. We've always computed gibibytes (GiB), so I've also fixed the unit (was previously GB).

  • Linux
  • macOS
  • Windows

@larsoner
Copy link
Member

larsoner commented Aug 13, 2024

One option to check win/macOS/Linux would be to add to the test something like assert re.match("[0-9]+ GiB", out) is not None in some sys_info test. It would ensure that on all CIs we get some estimate of the memory rather than ? GiB

@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 13, 2024

This is ready for review.

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

nice, LGTM!

@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 13, 2024

While I'm at it, I don't get any CPU name on my Mac, and at least for macOS I have an easy fix. Do you want me to include it in this PR or a new one?

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Works on Linux and I'll trust the CIs for the other OSes, thanks @cbrnr !

@larsoner larsoner enabled auto-merge (squash) August 13, 2024 19:53
@larsoner larsoner disabled auto-merge August 13, 2024 19:54
@larsoner
Copy link
Member

Sure, feel free to push here

@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 13, 2024

This gives

Apple M2 Pro (12 cores)

instead of

arm (12 cores)

I can try to get this to work on Linux and Windows tomorrow.

mne/utils/config.py Outdated Show resolved Hide resolved
@cbrnr cbrnr marked this pull request as draft August 13, 2024 20:06
@cbrnr cbrnr marked this pull request as ready for review August 14, 2024 06:30
@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 14, 2024

OK, this is now ready again. I've tested on all three platforms and got nice CPU strings as well as the correct memory size, but if you have hardware such as Intel-based Macs (I only tested on ARM-based Macs), please feel free to check the output of mne.sys_info()!

@cbrnr cbrnr added this to the 1.8 milestone Aug 14, 2024
@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 14, 2024

I'm using wmic on Windows, but I just found out that Microsoft is deprecating this tool. I'll see if I can replace it with some Powershell commands like they suggest.

Candidate replacements:

  • subprocess.check_output(["powershell.exe", "(Get-CimInstance Win32_PhysicalMemory | Measure-Object -Property capacity -Sum).sum"])
  • subprocess.check_output(["powershell.exe", "(Get-CimInstance Win32_Processor).Name"])

@hoechenberger
Copy link
Member

hoechenberger commented Aug 14, 2024

Can we not output GB instead of GiB? no-one except for a few CS and IT / tech-savvy folks will know what GiB is

@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 14, 2024

Can we not output GB instead of GiB? no-one except for a few CS and IT / tech-savvy folks will know what GiB is

Only if we really show GB (base 1000).

@hoechenberger
Copy link
Member

yes that's what I meant

@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 14, 2024

OK, but this changes the numbers then. Previously, we computed GiB but erroneously wrote GB.

@drammock
Copy link
Member

IMO, GiB is not that esoteric and people can always look it up with their search engine if they're not familiar. For me the first few hits are unrelated but hits 4 and 5 are "what is a GibiByte (GiB)?" and "GB vs GiB what's the difference?"

@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 14, 2024

IMO, GiB is not that esoteric and people can always look it up with their search engine if they're not familiar.

I agree! In principle, I don't care what we use as long as we use the correct unit though.

@larsoner
Copy link
Member

The tools I checked on Linux use GiB, the activity monitor or macOS says GB. No opinion from me on which to use but agree it would be nice to get the unit right.

Current approach appears to be failing on Windows CIs, though

https://github.com/mne-tools/mne-python/actions/runs/10386163230/job/28756877686?pr=12787#step:17:4174

https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=30965&view=logs&jobId=2bd7b19d-6351-5e7f-8417-63f327ab45bc&j=2bd7b19d-6351-5e7f-8417-63f327ab45bc&t=43b4c425-59e5-5cb3-0374-1f08e81b4191

@larsoner
Copy link
Member

I suggest we get this in early for 1.9 rather than just before 1.8. Might be good to have a bit more time to test it, I don't think this is a critical blocker, and we still need to converge a bit.

@cbrnr
Copy link
Contributor Author

cbrnr commented Aug 14, 2024

I mean, it's only the memory calculation which is currently failing. I could try and play around a bit more, but I'm also fine with moving it to 1.9.

@larsoner larsoner removed this from the 1.8 milestone Aug 14, 2024
@cbrnr
Copy link
Contributor Author

cbrnr commented Sep 9, 2024

This is now finally ready for review again. Everything seems to be working fine on all platforms.

Comment on lines 624 to 625
else:
total_memory = -1
Copy link
Member

Choose a reason for hiding this comment

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

why not raise here, so that later the exception will get caught and a ? will get displayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done!

@larsoner larsoner merged commit 828953e into mne-tools:main Sep 11, 2024
28 checks passed
@larsoner
Copy link
Member

Thanks @cbrnr !

@cbrnr cbrnr deleted the memory branch September 11, 2024 16:24
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.

Replace psutil.virtual_memory() with our own implementation?
4 participants