From aa29f14b246bc782bdba9ccab52f22c0cde484e1 Mon Sep 17 00:00:00 2001 From: Josh Levy-Kramer Date: Mon, 25 Apr 2022 13:27:02 +0100 Subject: [PATCH 1/8] Support concurrent processes building the same module --- cppimport/__init__.py | 24 ++++++++++++----------- cppimport/importer.py | 43 +++++++++++++++++++++++++++++++---------- environment.yml | 1 + setup.py | 2 +- tests/test_cppimport.py | 31 +++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 22 deletions(-) diff --git a/cppimport/__init__.py b/cppimport/__init__.py index aef1141..1d9a62c 100644 --- a/cppimport/__init__.py +++ b/cppimport/__init__.py @@ -8,9 +8,11 @@ from cppimport.find import _check_first_line_contains_cppimport settings = dict( - force_rebuild=False, + force_rebuild=False, # `force_rebuild` with multiple processes is not supported file_exts=[".cpp", ".c"], rtld_flags=ctypes.RTLD_LOCAL, + lock_suffix='.lock', + lock_timeout=10*60, remove_strict_prototypes=True, release_mode=os.getenv("CPPIMPORT_RELEASE_MODE", "0").lower() in ("true", "yes", "1"), @@ -60,16 +62,15 @@ def imp_from_filepath(filepath, fullname=None): is_build_needed, load_module, setup_module_data, - template_and_build, - try_load, + build_safely, ) - + filepath = os.path.abspath(filepath) if fullname is None: fullname = os.path.splitext(os.path.basename(filepath))[0] module_data = setup_module_data(fullname, filepath) - if is_build_needed(module_data) or not try_load(module_data): - template_and_build(filepath, module_data) - load_module(module_data) + if is_build_needed(module_data): + build_safely(filepath, module_data) + load_module(module_data) return module_data["module"] @@ -110,15 +111,16 @@ def build_filepath(filepath, fullname=None): from cppimport.importer import ( is_build_needed, setup_module_data, - template_and_build, + build_safely, + load_module, ) - + filepath = os.path.abspath(filepath) if fullname is None: fullname = os.path.splitext(os.path.basename(filepath))[0] module_data = setup_module_data(fullname, filepath) if is_build_needed(module_data): - template_and_build(filepath, module_data) - + build_safely(filepath, module_data) + load_module(module_data) # Return the path to the built module return module_data["ext_path"] diff --git a/cppimport/importer.py b/cppimport/importer.py index cf04f1a..e4c21b6 100644 --- a/cppimport/importer.py +++ b/cppimport/importer.py @@ -3,6 +3,9 @@ import os import sys import sysconfig +from contextlib import suppress +from time import time, sleep +import filelock import cppimport from cppimport.build_module import build_module @@ -12,6 +15,36 @@ logger = logging.getLogger(__name__) +def build_safely(filepath, module_data): + """Protect against race conditions when multiple processes executing `template_and_build`""" + binary_path = module_data['ext_path'] + lock_path = binary_path + cppimport.settings['lock_suffix'] + + build_completed = lambda: os.path.exists(binary_path) and is_checksum_valid(module_data) + + t = time() + + # Race to obtain the lock and build. Other processes can wait + while not build_completed() and time() - t < cppimport.settings['lock_timeout']: + try: + with filelock.FileLock(lock_path, timeout=1): + if build_completed(): + break + template_and_build(filepath, module_data) + except filelock.Timeout: + logging.debug(f'{os.getpid()}: Could not obtain lock') + sleep(1) + + if os.path.exists(lock_path): + with suppress(OSError): + os.remove(lock_path) + + if not build_completed(): + raise Exception( + f'Could not compile binary as lock already taken and timed out. Try increasing the timeout setting if ' + f'the build time is longer.') + + def template_and_build(filepath, module_data): logger.debug(f"Compiling {filepath}.") run_templating(module_data) @@ -77,13 +110,3 @@ def is_build_needed(module_data): logger.debug(f"Matching checksum for {module_data['filepath']} --> not compiling") return False - -def try_load(module_data): - try: - load_module(module_data) - return True - except ImportError as e: - logger.info( - f"ImportError during import with matching checksum: {e}. Trying to rebuild." - ) - return False diff --git a/environment.yml b/environment.yml index ca71dcb..0a90830 100644 --- a/environment.yml +++ b/environment.yml @@ -10,3 +10,4 @@ dependencies: - pytest - pytest-cov - pre-commit + - filelock diff --git a/setup.py b/setup.py index c19d9c4..8ac7601 100644 --- a/setup.py +++ b/setup.py @@ -6,7 +6,7 @@ use_scm_version={"version_scheme": "post-release"}, setup_requires=["setuptools_scm"], packages=["cppimport"], - install_requires=["mako", "pybind11"], + install_requires=["mako", "pybind11", "filelock"], zip_safe=False, name="cppimport", description="Import C++ files directly from Python!", diff --git a/tests/test_cppimport.py b/tests/test_cppimport.py index 8dd992a..acd355c 100644 --- a/tests/test_cppimport.py +++ b/tests/test_cppimport.py @@ -2,8 +2,11 @@ import copy import logging import os +import shutil import subprocess import sys +from multiprocessing import Process +from tempfile import TemporaryDirectory import cppimport import cppimport.build_module @@ -45,6 +48,15 @@ def subprocess_check(test_code, returncode=0): assert p.returncode == returncode +@contextlib.contextmanager +def tmp_dir(files=None): + files = files if files else [] + + with TemporaryDirectory() as tmp_path: + for f in files: + shutil.copyfile(f, os.path.join(tmp_path, f)) + yield tmp_path + def test_find_module_cpppath(): mymodule_loc = find_module_cpppath("mymodule") mymodule_dir = os.path.dirname(mymodule_loc) @@ -170,3 +182,22 @@ def test_import_hook(): cppimport.force_rebuild(False) hook_test + + +def test_multiple_processes(): + with tmp_dir(['hook_test.cpp']) as tmp_path: + test_code = f""" +import os; +os.chdir('{tmp_path}'); +import cppimport.import_hook; +import hook_test; + """ + processes = [Process(target=subprocess_check, args=(test_code, )) for i in range(100)] + + for p in processes: + p.start() + + for p in processes: + p.join() + + assert all(p.exitcode == 0 for p in processes) From 72a2e34f7454c9587bb2ef7b741b31580331cda9 Mon Sep 17 00:00:00 2001 From: Josh Levy-Kramer Date: Tue, 28 Jun 2022 13:57:11 +0100 Subject: [PATCH 2/8] Improve errors --- cppimport/importer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cppimport/importer.py b/cppimport/importer.py index e4c21b6..49a2abd 100644 --- a/cppimport/importer.py +++ b/cppimport/importer.py @@ -32,7 +32,7 @@ def build_safely(filepath, module_data): break template_and_build(filepath, module_data) except filelock.Timeout: - logging.debug(f'{os.getpid()}: Could not obtain lock') + logging.debug(f'Could not obtain lock (pid {os.getpid()})') sleep(1) if os.path.exists(lock_path): @@ -42,7 +42,7 @@ def build_safely(filepath, module_data): if not build_completed(): raise Exception( f'Could not compile binary as lock already taken and timed out. Try increasing the timeout setting if ' - f'the build time is longer.') + f'the build time is longer (pid {os.getpid()}).') def template_and_build(filepath, module_data): From 53dfaffc51bb8afd32b6190750a07c49e0404c89 Mon Sep 17 00:00:00 2001 From: Josh Levy-Kramer Date: Tue, 28 Jun 2022 13:58:48 +0100 Subject: [PATCH 3/8] Improve tests --- tests/test_cppimport.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_cppimport.py b/tests/test_cppimport.py index acd355c..fdc3bb0 100644 --- a/tests/test_cppimport.py +++ b/tests/test_cppimport.py @@ -50,13 +50,18 @@ def subprocess_check(test_code, returncode=0): @contextlib.contextmanager def tmp_dir(files=None): + """Create a temporary directory and copy `files` into it. `files` can also include directories.""" files = files if files else [] with TemporaryDirectory() as tmp_path: for f in files: - shutil.copyfile(f, os.path.join(tmp_path, f)) + if os.path.isdir(f): + shutil.copytree(f, os.path.join(tmp_path, os.path.basename(f))) + else: + shutil.copyfile(f, os.path.join(tmp_path, os.path.basename(f))) yield tmp_path + def test_find_module_cpppath(): mymodule_loc = find_module_cpppath("mymodule") mymodule_dir = os.path.dirname(mymodule_loc) From 94e0709e0f0d3b4feee10da40cee3e449c64fc85 Mon Sep 17 00:00:00 2001 From: Josh Levy-Kramer Date: Fri, 8 Jul 2022 13:00:50 +0100 Subject: [PATCH 4/8] Address comments --- .github/workflows/test.yml | 2 +- README.md | 16 ++++++++++++++++ cppimport/__init__.py | 3 ++- cppimport/importer.py | 13 +++++++++++++ 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dfe31d8..0ff6199 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,6 +1,6 @@ name: Test -on: [push] +on: [push, pull_request] jobs: test: diff --git a/README.md b/README.md index b30e896..6aad20b 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,22 @@ cppimport.settings['force_rebuild'] = True And if this is a common occurence, I would love to hear your use case and why the combination of the checksum, `cfg['dependencies']` and `cfg['sources']` is insufficient! +Note that `force_rebuild` does not work when importing the module concurrently. + +### Can I import my model concurrently? + +It's safe to use `cppimport` to import a module concurrently using multiple threads, processes or even machines! + +Before building a module, `cppimport` obtains a lockfile preventing other processors from building it at the same time - this prevents clashes that can lead to failure. +Other processes will wait maximum 10 mins until the first process has built the module and load it. If your module does not build within 10 mins then it will timeout. +You can increase the timeout time in the settings: + +```python +cppimport.settings['lock_timeout'] = 10*60 # 10 mins +``` + +You should not use `force_rebuild` when importing concurrently. + ### How can I get information about filepaths in the configuration block? The module name is available as the `fullname` variable and the C++ module file is available as `filepath`. For example, diff --git a/cppimport/__init__.py b/cppimport/__init__.py index 1d9a62c..96f29d9 100644 --- a/cppimport/__init__.py +++ b/cppimport/__init__.py @@ -63,12 +63,13 @@ def imp_from_filepath(filepath, fullname=None): load_module, setup_module_data, build_safely, + try_load, ) filepath = os.path.abspath(filepath) if fullname is None: fullname = os.path.splitext(os.path.basename(filepath))[0] module_data = setup_module_data(fullname, filepath) - if is_build_needed(module_data): + if is_build_needed(module_data) or not try_load(module_data): build_safely(filepath, module_data) load_module(module_data) return module_data["module"] diff --git a/cppimport/importer.py b/cppimport/importer.py index 49a2abd..3388771 100644 --- a/cppimport/importer.py +++ b/cppimport/importer.py @@ -110,3 +110,16 @@ def is_build_needed(module_data): logger.debug(f"Matching checksum for {module_data['filepath']} --> not compiling") return False + +def try_load(module_data): + """Try loading the module to test if it's not corrupt and for the correct architecture""" + try: + load_module(module_data) + return True + except ImportError as e: + logger.info( + f"ImportError during import with matching checksum: {e}. Trying to rebuild." + ) + with suppress(OSError): + os.remove(module_data["fullname"]) + return False From 3f9c5f75ed2a9334696c407cac43c0d0ddeef9d5 Mon Sep 17 00:00:00 2001 From: Ben Thompson Date: Fri, 8 Jul 2022 10:24:12 -0400 Subject: [PATCH 5/8] Run precommit, plus other small edits. --- cppimport/__init__.py | 18 +++++++++++++----- cppimport/importer.py | 32 ++++++++++++++++++++++---------- tests/test_cppimport.py | 9 ++++++--- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/cppimport/__init__.py b/cppimport/__init__.py index 96f29d9..0690538 100644 --- a/cppimport/__init__.py +++ b/cppimport/__init__.py @@ -11,8 +11,8 @@ force_rebuild=False, # `force_rebuild` with multiple processes is not supported file_exts=[".cpp", ".c"], rtld_flags=ctypes.RTLD_LOCAL, - lock_suffix='.lock', - lock_timeout=10*60, + lock_suffix=".lock", + lock_timeout=10 * 60, remove_strict_prototypes=True, release_mode=os.getenv("CPPIMPORT_RELEASE_MODE", "0").lower() in ("true", "yes", "1"), @@ -59,16 +59,23 @@ def imp_from_filepath(filepath, fullname=None): module : the compiled and loaded Python extension module """ from cppimport.importer import ( + build_safely, is_build_needed, load_module, setup_module_data, - build_safely, try_load, ) + filepath = os.path.abspath(filepath) if fullname is None: fullname = os.path.splitext(os.path.basename(filepath))[0] module_data = setup_module_data(fullname, filepath) + # The call to try_load is necessary here because there are times when the + # only evidence a rebuild is needed comes from attempting to load an + # existing extension module. For example, if the extension was built on a + # different architecture or with different Python headers and will produce + # an error when loaded, then the load will fail. In that situation, we will + # need to rebuild. if is_build_needed(module_data) or not try_load(module_data): build_safely(filepath, module_data) load_module(module_data) @@ -110,11 +117,12 @@ def build_filepath(filepath, fullname=None): ext_path : the path to the compiled extension. """ from cppimport.importer import ( - is_build_needed, - setup_module_data, build_safely, + is_build_needed, load_module, + setup_module_data, ) + filepath = os.path.abspath(filepath) if fullname is None: fullname = os.path.splitext(os.path.basename(filepath))[0] diff --git a/cppimport/importer.py b/cppimport/importer.py index 3388771..bd5dcbd 100644 --- a/cppimport/importer.py +++ b/cppimport/importer.py @@ -4,7 +4,8 @@ import sys import sysconfig from contextlib import suppress -from time import time, sleep +from time import sleep, time + import filelock import cppimport @@ -16,23 +17,31 @@ def build_safely(filepath, module_data): - """Protect against race conditions when multiple processes executing `template_and_build`""" - binary_path = module_data['ext_path'] - lock_path = binary_path + cppimport.settings['lock_suffix'] + """Protect against race conditions when multiple processes executing + `template_and_build`""" + binary_path = module_data["ext_path"] + lock_path = binary_path + cppimport.settings["lock_suffix"] - build_completed = lambda: os.path.exists(binary_path) and is_checksum_valid(module_data) + def build_completed(): + return os.path.exists(binary_path) and is_checksum_valid(module_data) t = time() # Race to obtain the lock and build. Other processes can wait - while not build_completed() and time() - t < cppimport.settings['lock_timeout']: + while not build_completed() and time() - t < cppimport.settings["lock_timeout"]: try: with filelock.FileLock(lock_path, timeout=1): if build_completed(): break template_and_build(filepath, module_data) except filelock.Timeout: - logging.debug(f'Could not obtain lock (pid {os.getpid()})') + logging.debug(f"Could not obtain lock (pid {os.getpid()})") + if cppimport.settings["force_rebuild"]: + raise ValueError( + "force_build must be False to build concurrently." + "This process failed to claim a filelock indicating that" + " a concurrent build is in progress" + ) sleep(1) if os.path.exists(lock_path): @@ -41,8 +50,10 @@ def build_safely(filepath, module_data): if not build_completed(): raise Exception( - f'Could not compile binary as lock already taken and timed out. Try increasing the timeout setting if ' - f'the build time is longer (pid {os.getpid()}).') + f"Could not compile binary as lock already taken and timed out." + f" Try increasing the timeout setting if " + f"the build time is longer (pid {os.getpid()})." + ) def template_and_build(filepath, module_data): @@ -112,7 +123,8 @@ def is_build_needed(module_data): def try_load(module_data): - """Try loading the module to test if it's not corrupt and for the correct architecture""" + """Try loading the module to test if it's not corrupt and for the correct + architecture""" try: load_module(module_data) return True diff --git a/tests/test_cppimport.py b/tests/test_cppimport.py index fdc3bb0..fc408fa 100644 --- a/tests/test_cppimport.py +++ b/tests/test_cppimport.py @@ -50,7 +50,8 @@ def subprocess_check(test_code, returncode=0): @contextlib.contextmanager def tmp_dir(files=None): - """Create a temporary directory and copy `files` into it. `files` can also include directories.""" + """Create a temporary directory and copy `files` into it. `files` can also + include directories.""" files = files if files else [] with TemporaryDirectory() as tmp_path: @@ -190,14 +191,16 @@ def test_import_hook(): def test_multiple_processes(): - with tmp_dir(['hook_test.cpp']) as tmp_path: + with tmp_dir(["tests/hook_test.cpp"]) as tmp_path: test_code = f""" import os; os.chdir('{tmp_path}'); import cppimport.import_hook; import hook_test; """ - processes = [Process(target=subprocess_check, args=(test_code, )) for i in range(100)] + processes = [ + Process(target=subprocess_check, args=(test_code,)) for i in range(100) + ] for p in processes: p.start() From bb39c23948d58f5b21f608d955758652077f66eb Mon Sep 17 00:00:00 2001 From: Ben Thompson Date: Fri, 8 Jul 2022 10:39:59 -0400 Subject: [PATCH 6/8] Allow for OSError when checking if built module exists. --- cppimport/importer.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cppimport/importer.py b/cppimport/importer.py index bd5dcbd..7119d65 100644 --- a/cppimport/importer.py +++ b/cppimport/importer.py @@ -23,7 +23,10 @@ def build_safely(filepath, module_data): lock_path = binary_path + cppimport.settings["lock_suffix"] def build_completed(): - return os.path.exists(binary_path) and is_checksum_valid(module_data) + try: + return os.path.exists(binary_path) and is_checksum_valid(module_data) + except OSError: + return False t = time() From 546b5b2f5904ddd72193ce726a887577a42900c8 Mon Sep 17 00:00:00 2001 From: Ben Thompson Date: Fri, 8 Jul 2022 10:56:17 -0400 Subject: [PATCH 7/8] Catch OSError in checksum trailer check. --- cppimport/checksum.py | 3 +++ cppimport/importer.py | 5 +---- tests/test_cppimport.py | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cppimport/checksum.py b/cppimport/checksum.py index 6e717ac..f991fd9 100644 --- a/cppimport/checksum.py +++ b/cppimport/checksum.py @@ -45,6 +45,9 @@ def _load_checksum_trailer(module_data): except FileNotFoundError: logger.info("Failed to find compiled extension; rebuilding.") return None, None + except OSError: + logger.info("Checksum trailer invalid. Rebuilding.") + return None, None try: deps, old_checksum = json.loads(json_s) diff --git a/cppimport/importer.py b/cppimport/importer.py index 7119d65..bd5dcbd 100644 --- a/cppimport/importer.py +++ b/cppimport/importer.py @@ -23,10 +23,7 @@ def build_safely(filepath, module_data): lock_path = binary_path + cppimport.settings["lock_suffix"] def build_completed(): - try: - return os.path.exists(binary_path) and is_checksum_valid(module_data) - except OSError: - return False + return os.path.exists(binary_path) and is_checksum_valid(module_data) t = time() diff --git a/tests/test_cppimport.py b/tests/test_cppimport.py index fc408fa..a81c8fd 100644 --- a/tests/test_cppimport.py +++ b/tests/test_cppimport.py @@ -43,8 +43,10 @@ def subprocess_check(test_code, returncode=0): stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) - print(p.stdout.decode("utf-8")) - print(p.stderr.decode("utf-8")) + if len(p.stdout) > 0: + print(p.stdout.decode("utf-8")) + if len(p.stderr) > 0: + print(p.stderr.decode("utf-8")) assert p.returncode == returncode From 7213cc44506aa53836b4d9e75cd1f2bd676e4c38 Mon Sep 17 00:00:00 2001 From: Ben Thompson Date: Fri, 8 Jul 2022 11:00:24 -0400 Subject: [PATCH 8/8] buffering=0 for checksum trailer save. --- cppimport/checksum.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cppimport/checksum.py b/cppimport/checksum.py index f991fd9..57b8a07 100644 --- a/cppimport/checksum.py +++ b/cppimport/checksum.py @@ -82,7 +82,7 @@ def _save_checksum_trailer(module_data, dep_filepaths, cur_checksum): # legal (see e.g. https://stackoverflow.com/questions/10106447). dump = json.dumps([dep_filepaths, cur_checksum]).encode("ascii") dump += _FMT.pack(len(dump), _TAG) - with open(module_data["ext_path"], "ab") as file: + with open(module_data["ext_path"], "ab", buffering=0) as file: file.write(dump)