-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
TST: machinery for tests requiring large memory + lapack64 smoketest #15021
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
Conversation
Azure failures seems spurious ("[NuGet] The operation has timed out" when installing mingw) |
Thanks Pauli. |
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.
Mostly looks good, one bug and some nits
with open('/proc/meminfo', 'r') as f: | ||
for line in f: | ||
p = line.split() | ||
info[p[0].strip(':').lower()] = float(p[1]) * 1e3 |
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.
info[p[0].strip(':').lower()] = float(p[1]) * 1e3 | |
info[p[0].strip(':').lower()] = int(p[1]) * 1024 |
Based on https://lore.kernel.org/patchwork/patch/666444/, this measurement is in KiB
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.
Ok, thanks. (follow-up in gh-15052)
'k': 1e3, 'm': 1e6, 'g': 1e9, 't': 1e12, | ||
'kb': 1e3, 'mb': 1e6, 'gb': 1e9, 'tb': 1e12} |
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.
nit: consider including kib
, MiB
, etc
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.
Added
|
||
def _parse_size(size_str): | ||
"""Convert memory size strings ('12 GB' etc.) to float""" | ||
suffixes = {'': 1.0, 'b': 1.0, |
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.
nit: Perhaps for brevity:
suffixes = {'': 1.0, 'k': 1e3, 'm': 1e6, 'g': 1e9, 't': 1e12}
suffixes.update({k + 'b': v for k, v in suffixes.items()}) # allow a trailing b
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.
After adding kib/et al, it seemed cleaner to not do this.
mem_free = -1 | ||
else: | ||
msg = '{0} GB memory required, but {1} GB available'.format( | ||
free_bytes/1e9, mem_free/1e9) |
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.
nit: could use _ArrayMemoryError._size_to_string
here to format numbers as prefixed bytes.
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.
Didn't change this: corner-case message in test suite, and probably would have involved refactoring the utility function out from _ArrayMemoryError
.
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.
Fair
env_var, exc)) | ||
|
||
msg = ('{0} GB memory required, but environment variable ' | ||
'NPY_AVAILABLE_MEM={1} set'.format( |
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.
'NPY_AVAILABLE_MEM={1} set'.format( | |
'NPY_AVAILABLE_MEM="{1} GB" set'.format( |
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.
{1} is the string value of the env var here
m = size_re.match(size_str.lower()) | ||
if not m or m.group(2) not in suffixes: | ||
raise ValueError("value {!r} not a valid size".format(size_str)) | ||
return float(m.group(1)) * suffixes[m.group(2)] |
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.
nit: sizes are ints,
return float(m.group(1)) * suffixes[m.group(2)] | |
return int(float(m.group(1)) * suffixes[m.group(2)]) |
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.
ok
import sys | ||
import os | ||
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.
Why this change?
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.
Added by mistake.
@charris, looks like you merged while I was reviewing. It's unfortunate github doesn't provide an easy way to collect the diffs above into a patch once it's merged. |
@eric-wieser The changes look small, just make a new PR. I was kinda counting on that anyway :) |
Implement test suite machinery for (automatically) skipping tests that require large amounts of memory.
Also, add a companion test for gh-15012, which checks 64-bit LAPACK works as expected.
(I ended up adding
np.dot
check instead of theeigh
check, because although theeigh
one seems to take less memory, the runtime seems to be > 20min so it's not suitable here.)