From dfbb67796b51a4399e9dddf4447b20b5ac18e006 Mon Sep 17 00:00:00 2001 From: Joe Zuntz Date: Wed, 24 Jun 2020 12:55:23 +0100 Subject: [PATCH 1/3] add python_paths option to config --- ceci/main.py | 84 +++++++++++++++++------------ ceci/utils.py | 42 +++++++++++++++ tests/test_python_paths.py | 106 +++++++++++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+), 35 deletions(-) create mode 100644 ceci/utils.py create mode 100644 tests/test_python_paths.py diff --git a/ceci/main.py b/ceci/main.py index e0eb549..9d3ddad 100644 --- a/ceci/main.py +++ b/ceci/main.py @@ -5,6 +5,7 @@ import subprocess from . import pipeline from .sites import load, set_default_site, get_default_site +from .utils import extra_paths # Add the current dir to the path - often very useful sys.path.append(os.getcwd()) @@ -106,8 +107,6 @@ def run(pipeline_config_filename, extra_config=None, dry_run=False): "resume": pipe_config["resume"], } - for module in modules: - __import__(module) # Choice of actual pipeline type to run if dry_run: @@ -121,39 +120,54 @@ def run(pipeline_config_filename, extra_config=None, dry_run=False): else: raise ValueError("Unknown pipeline launcher {launcher_name}") - # Run the pre-script. Since it's an error for this to fail (because - # it is useful as a validation check) then we raise an error if it - # fails using check_call. - if pre_script and not dry_run: - subprocess.check_call(pre_script.split() + script_args, shell=True) - - # Create and run the pipeline - p = pipeline_class(stages, launcher_config) - status = p.run(inputs, run_config, stages_config) - - # The load command above changes the default site. - # So that this function doesn't confuse later things, - # reset that site now. - set_default_site(default_site) - - if status: - return status - - # Run the post-script. There seems less point raising an actual error - # here, as the pipeline is complete, so we just issue a warning and - # return a status code to the caller (e.g. to the command line). - # Thoughts on this welcome. - if post_script and not dry_run: - return_code = subprocess.call(post_script.split() + script_args, shell=True) - if return_code: - sys.stderr.write( - f"\nWARNING: The post-script command {post_script} " - "returned error status {return_code}\n\n" - ) - return return_code - # Otherwise everything must have gone fine. - else: - return status + + + + paths = pipe_config.get("python_paths", []) + if isinstance(paths, str): + paths = paths.split() + + # temporarily add the paths to sys.path, + # but remove them at the end + with extra_paths(paths): + + for module in modules: + __import__(module) + + # Run the pre-script. Since it's an error for this to fail (because + # it is useful as a validation check) then we raise an error if it + # fails using check_call. + if pre_script and not dry_run: + subprocess.check_call(pre_script.split() + script_args, shell=True) + + # Create and run the pipeline + try: + p = pipeline_class(stages, launcher_config) + status = p.run(inputs, run_config, stages_config) + finally: + # The load command above changes the default site. + # So that this function doesn't confuse later things, + # reset that site now. + set_default_site(default_site) + + if status: + return status + + # Run the post-script. There seems less point raising an actual error + # here, as the pipeline is complete, so we just issue a warning and + # return a status code to the caller (e.g. to the command line). + # Thoughts on this welcome. + if post_script and not dry_run: + return_code = subprocess.call(post_script.split() + script_args, shell=True) + if return_code: + sys.stderr.write( + f"\nWARNING: The post-script command {post_script} " + "returned error status {return_code}\n\n" + ) + return return_code + # Otherwise everything must have gone fine. + else: + return status def override_config(config, extra): diff --git a/ceci/utils.py b/ceci/utils.py new file mode 100644 index 0000000..e039a40 --- /dev/null +++ b/ceci/utils.py @@ -0,0 +1,42 @@ +from contextlib import contextmanager +import sys + +@contextmanager +def extra_paths(paths, start=True): + if isinstance(paths, str): + paths = paths.split() + + for path in paths: + if start: + sys.path.insert(0, path) + else: + sys.path.append(path) + + try: + yield + finally: + for path in paths: + if start: + sys.path.remove(path) + else: + remove_last(sys.path, path) + +def remove_last(lst, item): + """ + Removes (in-place) the last instance of item from the list lst. + Raises ValueError if item is not in list + + Parameters + ---------- + lst: List + A list of anything + item: object + Item to be removed + + Returns + ------- + None + """ + tmp = lst[::-1] + tmp.remove(item) + lst[:] = tmp[::-1] diff --git a/tests/test_python_paths.py b/tests/test_python_paths.py new file mode 100644 index 0000000..0ca2e8f --- /dev/null +++ b/tests/test_python_paths.py @@ -0,0 +1,106 @@ +import sys +from ceci.main import run +from ceci.utils import remove_last, extra_paths +import pytest + +def test_remove_item(): + l = list('abcdea') + remove_last(l, 'a') + assert l == list('abcde') + + l = list('abcde') + remove_last(l, 'b') + assert l == list('acde') + + with pytest.raises(ValueError): + remove_last([1, 2 ,3], 4) + +class MyError(Exception): + pass + +def test_extra_paths(): + p = 'xxx111yyy222' + orig_path = sys.path[:] + + # check path is put in + with extra_paths(p): + assert sys.path[0] == p + + # check everything back to normal + # after with statement + assert p not in sys.path + assert sys.path == orig_path + + # check that an exception does not interfere + # with this + try: + with extra_paths(p): + assert sys.path[0] == p + raise MyError("x") + except MyError: + pass + + assert p not in sys.path + assert sys.path == orig_path + + + # now putting the item at the end not the start + with extra_paths(p, start=False): + assert sys.path[-1] == p + + assert p not in sys.path + assert sys.path == orig_path + + try: + with extra_paths(p, start=False): + assert sys.path[-1] == p + raise MyError("x") + except MyError: + pass + + assert p not in sys.path + assert sys.path == orig_path + + # now agan with a list of paths + p = ['xxx111yyy222', 'aaa222333'] + with extra_paths(p): + assert sys.path[0] == p[1] + assert sys.path[1] == p[0] + + assert p not in sys.path + assert sys.path == orig_path + + try: + with extra_paths(p): + assert sys.path[0] == p[1] + assert sys.path[1] == p[0] + raise MyError("x") + except MyError: + pass + + assert p not in sys.path + assert sys.path == orig_path + + + + # now agan with a list of paths, at the end + p = ['xxx111yyy222', 'aaa222333'] + with extra_paths(p, start=False): + assert sys.path[-1] == p[1] + assert sys.path[-2] == p[0] + + assert p not in sys.path + assert sys.path == orig_path + + try: + with extra_paths(p, start=False): + assert sys.path[-1] == p[1] + assert sys.path[-2] == p[0] + raise MyError("x") + except MyError: + pass + + assert p not in sys.path + assert sys.path == orig_path + + From 3799e7a0481f91b3b8ac07a3b5e567248505f9a5 Mon Sep 17 00:00:00 2001 From: Joe Zuntz Date: Thu, 25 Jun 2020 18:21:10 +0100 Subject: [PATCH 2/3] add pytest to install --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index c93a818..eb67f55 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,7 @@ python: - "3.8" install: + - pip install --upgrade pytest codecov - pip install .[test,cwl,parsl] # Test three different pipelines From 8ea0c54e3278e2edd5816c7d29cd9bf409171b47 Mon Sep 17 00:00:00 2001 From: Joe Zuntz Date: Mon, 29 Jun 2020 10:43:47 +0100 Subject: [PATCH 3/3] add more tests and allow user to modify path during with statement --- ceci/utils.py | 21 +++++++++++++++++---- tests/test_python_paths.py | 27 ++++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/ceci/utils.py b/ceci/utils.py index e039a40..21a74ea 100644 --- a/ceci/utils.py +++ b/ceci/utils.py @@ -3,23 +3,36 @@ @contextmanager def extra_paths(paths, start=True): + # allow passing a single path or + # a list of them if isinstance(paths, str): paths = paths.split() + # On enter, add paths to sys.path, + # either the start or the end depending + # on the start argument for path in paths: if start: sys.path.insert(0, path) else: sys.path.append(path) + # Return control to caller try: yield + # On exit, remove the paths finally: for path in paths: - if start: - sys.path.remove(path) - else: - remove_last(sys.path, path) + try: + if start: + sys.path.remove(path) + else: + remove_last(sys.path, path) + # If e.g. user has already done this + # manually for some reason then just + # skip + except ValueError: + pass def remove_last(lst, item): """ diff --git a/tests/test_python_paths.py b/tests/test_python_paths.py index 0ca2e8f..d8f4a93 100644 --- a/tests/test_python_paths.py +++ b/tests/test_python_paths.py @@ -67,7 +67,8 @@ def test_extra_paths(): assert sys.path[0] == p[1] assert sys.path[1] == p[0] - assert p not in sys.path + for p1 in p: + assert p1 not in sys.path assert sys.path == orig_path try: @@ -78,7 +79,8 @@ def test_extra_paths(): except MyError: pass - assert p not in sys.path + for p1 in p: + assert p1 not in sys.path assert sys.path == orig_path @@ -89,7 +91,8 @@ def test_extra_paths(): assert sys.path[-1] == p[1] assert sys.path[-2] == p[0] - assert p not in sys.path + for p1 in p: + assert p1 not in sys.path assert sys.path == orig_path try: @@ -103,4 +106,22 @@ def test_extra_paths(): assert p not in sys.path assert sys.path == orig_path + # check that if the user removes the path + # themselves then it is okay + p = ['xxx111yyy222', 'aaa222333'] + with extra_paths(p, start=True): + sys.path.remove('xxx111yyy222') + + assert sys.path == orig_path + # check only one copy is removed + sys.path.append("aaa") + tmp_paths = sys.path[:] + p = "aaa" + with extra_paths(p, start=True): + pass + + assert sys.path == tmp_paths + + with extra_paths(p, start=False): + pass