Skip to content

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

Merged
merged 3 commits into from
Dec 2, 2019

Conversation

pv
Copy link
Member

@pv pv commented Dec 1, 2019

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 the eigh check, because although the eigh one seems to take less memory, the runtime seems to be > 20min so it's not suitable here.)

@pv
Copy link
Member Author

pv commented Dec 1, 2019

Azure failures seems spurious ("[NuGet] The operation has timed out" when installing mingw)

@charris
Copy link
Member

charris commented Dec 2, 2019

Thanks Pauli.

Copy link
Member

@eric-wieser eric-wieser left a 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Member Author

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)

Comment on lines +2421 to +2422
'k': 1e3, 'm': 1e6, 'g': 1e9, 't': 1e12,
'kb': 1e3, 'mb': 1e6, 'gb': 1e9, 'tb': 1e12}
Copy link
Member

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

Copy link
Member Author

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,
Copy link
Member

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

Copy link
Member Author

@pv pv Dec 4, 2019

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)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'NPY_AVAILABLE_MEM={1} set'.format(
'NPY_AVAILABLE_MEM="{1} GB" set'.format(

Copy link
Member Author

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)]
Copy link
Member

Choose a reason for hiding this comment

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

nit: sizes are ints,

Suggested change
return float(m.group(1)) * suffixes[m.group(2)]
return int(float(m.group(1)) * suffixes[m.group(2)])

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

Comment on lines +18 to +21
import sys
import os
import re

Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added by mistake.

@eric-wieser
Copy link
Member

@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.

@charris
Copy link
Member

charris commented Dec 2, 2019

@eric-wieser The changes look small, just make a new PR. I was kinda counting on that anyway :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants