From 4309b07edeac500affd0183c75a2157b7c32f72e Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 07:59:42 -0700 Subject: [PATCH 01/17] AIP-6643: Add in log file output per test --- .gitignore | 3 ++- metaflow/plugins/kfp/tests/conftest.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 31afa021418..71268bbfc71 100644 --- a/.gitignore +++ b/.gitignore @@ -30,4 +30,5 @@ main.js.map .idea .coverage -.coverage.* \ No newline at end of file +.coverage.* +pytest-logs \ No newline at end of file diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index f19753afd1e..f8d7d199428 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -1,3 +1,7 @@ +import pytest +from pathlib import Path + + def pytest_addoption(parser): """ The image on Artifactory that corresponds to the currently @@ -7,3 +11,15 @@ def pytest_addoption(parser): parser.addoption( "--opsgenie-api-token", dest="opsgenie_api_token", action="store", default=None ) + + +@pytest.hookimpl(hookwrapper=True, tryfirst=True) +def pytest_runtest_setup(item): + """Emit a log file per test to make it easier to debug in failure scenarios. + + Sourced from: https://stackoverflow.com/a/64480499 + """ + logging_plugin = item.config.pluginmanager.get_plugin("logging-plugin") + filename = Path('pytest-logs', f"{item._request.node.name}.log") + logging_plugin.set_log_path(str(filename)) + yield From 163043164e004250770a6324ffad289bd4a59929 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 08:12:59 -0700 Subject: [PATCH 02/17] AIP-6643: Pass in public_dir so logs can be outputted per test. Also temporarily disable most tests to expedite debugging --- metaflow/plugins/kfp/tests/conftest.py | 9 +++++++-- metaflow/plugins/kfp/tests/run_integration_tests.py | 4 ++++ metaflow/plugins/kfp/tests/setup.cfg | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index f8d7d199428..2e636a43c00 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -11,15 +11,20 @@ def pytest_addoption(parser): parser.addoption( "--opsgenie-api-token", dest="opsgenie_api_token", action="store", default=None ) + parser.addoption("--public-directory", dest="public_dir", action="store", default='') @pytest.hookimpl(hookwrapper=True, tryfirst=True) -def pytest_runtest_setup(item): +def pytest_runtest_setup(item, pytestconfig): """Emit a log file per test to make it easier to debug in failure scenarios. Sourced from: https://stackoverflow.com/a/64480499 """ logging_plugin = item.config.pluginmanager.get_plugin("logging-plugin") - filename = Path('pytest-logs', f"{item._request.node.name}.log") + filename = Path( + pytestconfig.getoption('public_dir'), + 'pytest-logs', + f"{item._request.node.name}.log" + ) logging_plugin.set_log_path(str(filename)) yield diff --git a/metaflow/plugins/kfp/tests/run_integration_tests.py b/metaflow/plugins/kfp/tests/run_integration_tests.py index e06cd5fe60f..65b69902c85 100644 --- a/metaflow/plugins/kfp/tests/run_integration_tests.py +++ b/metaflow/plugins/kfp/tests/run_integration_tests.py @@ -131,6 +131,7 @@ def test_s3_sensor_flow(pytestconfig) -> None: # This test ensures that a flow fails correctly, # and when it fails, an OpsGenie email is sent. +@pytest.mark.skip def test_error_and_opsgenie_alert(pytestconfig) -> None: raise_error_flow_cmd: str = ( f"{_python()} flows/raise_error_flow.py --datastore=s3 kfp run " @@ -204,6 +205,7 @@ def test_error_and_opsgenie_alert(pytestconfig) -> None: return +@pytest.mark.skip @pytest.mark.parametrize("flow_file_path", obtain_flow_file_paths("flows")) def test_flows(pytestconfig, flow_file_path: str) -> None: full_path: str = join("flows", flow_file_path) @@ -316,6 +318,7 @@ def get_compiled_yaml(compile_to_yaml_cmd, yaml_file_path) -> Dict[str, str]: return flow_yaml +@pytest.mark.skip def test_kubernetes_service_account_compile_only() -> None: service_account = "test-service-account" with tempfile.TemporaryDirectory() as yaml_tmp_dir: @@ -332,6 +335,7 @@ def test_kubernetes_service_account_compile_only() -> None: assert flow_yaml["spec"]["serviceAccountName"] == service_account +@pytest.mark.skip def test_toleration_and_affinity_compile_only() -> None: step_templates: Dict[str, str] = {} with tempfile.TemporaryDirectory() as yaml_tmp_dir: diff --git a/metaflow/plugins/kfp/tests/setup.cfg b/metaflow/plugins/kfp/tests/setup.cfg index ff518713bf1..9c3dbe90eec 100644 --- a/metaflow/plugins/kfp/tests/setup.cfg +++ b/metaflow/plugins/kfp/tests/setup.cfg @@ -13,7 +13,7 @@ exclude_lines = # pytest [tool:pytest] -addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html +addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --public-dir /home/zservice/public [html] -directory = /home/zservice/public +directory = /home/zservice/public/coverage From 8fa8ffc93c82075e0d1c7aa2a7b28fcd0cc30b07 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 08:27:43 -0700 Subject: [PATCH 03/17] AIP-6643: Pass in public_dir so logs can be outputted per test. Also temporarily disable most tests to expedite debugging --- metaflow/plugins/kfp/tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index 2e636a43c00..9175e9d6af1 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -11,7 +11,7 @@ def pytest_addoption(parser): parser.addoption( "--opsgenie-api-token", dest="opsgenie_api_token", action="store", default=None ) - parser.addoption("--public-directory", dest="public_dir", action="store", default='') + parser.addoption("--public-dir", dest="public_dir", action="store", default='') @pytest.hookimpl(hookwrapper=True, tryfirst=True) From e5581dbf82d0d0f71a19ceafae685a49fbfb5f32 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 08:42:55 -0700 Subject: [PATCH 04/17] AIP-6643: Pass in public_dir so logs can be outputted per test. Also temporarily disable most tests to expedite debugging --- .gitlab-ci.yml | 2 +- metaflow/plugins/kfp/tests/setup.cfg | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e1d84ed9f74..97fe4edcdb8 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -72,7 +72,7 @@ stages: ${BUILT_IMAGE_FULL_PATH} bash -c " cd /home/zservice/metaflow/metaflow/plugins/kfp/tests && - python -m pytest -s -n 3 run_integration_tests.py --image ${BUILT_IMAGE_FULL_PATH} --opsgenie-api-token ${OPSGENIE_API_TOKEN} --cov-config=setup.cfg + python -m pytest -s -n 3 run_integration_tests.py --image ${BUILT_IMAGE_FULL_PATH} --opsgenie-api-token ${OPSGENIE_API_TOKEN} --cov-config=setup.cfg --public-dir /home/zservice/public " artifacts: when: always diff --git a/metaflow/plugins/kfp/tests/setup.cfg b/metaflow/plugins/kfp/tests/setup.cfg index 9c3dbe90eec..26f1e5666cc 100644 --- a/metaflow/plugins/kfp/tests/setup.cfg +++ b/metaflow/plugins/kfp/tests/setup.cfg @@ -13,7 +13,7 @@ exclude_lines = # pytest [tool:pytest] -addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --public-dir /home/zservice/public +addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html [html] directory = /home/zservice/public/coverage From be189ca6ab96fc04c7f3c616e818241a887bf514 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 09:02:53 -0700 Subject: [PATCH 05/17] AIP-6643: Pass in public_dir so logs can be outputted per test. Also temporarily disable most tests to expedite debugging --- metaflow/plugins/kfp/tests/conftest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index 9175e9d6af1..1e456baf321 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -15,14 +15,14 @@ def pytest_addoption(parser): @pytest.hookimpl(hookwrapper=True, tryfirst=True) -def pytest_runtest_setup(item, pytestconfig): +def pytest_runtest_setup(item): """Emit a log file per test to make it easier to debug in failure scenarios. Sourced from: https://stackoverflow.com/a/64480499 """ logging_plugin = item.config.pluginmanager.get_plugin("logging-plugin") filename = Path( - pytestconfig.getoption('public_dir'), + item.config.getoption('public_dir'), 'pytest-logs', f"{item._request.node.name}.log" ) From 0ccfb9d759cdaa55c959cb8fe11da432f53b6028 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 09:32:48 -0700 Subject: [PATCH 06/17] AIP-6643: Pass in public_dir so logs can be outputted per test. Also temporarily disable most tests to expedite debugging --- metaflow/plugins/kfp/tests/setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metaflow/plugins/kfp/tests/setup.cfg b/metaflow/plugins/kfp/tests/setup.cfg index 26f1e5666cc..d2bc96bbb86 100644 --- a/metaflow/plugins/kfp/tests/setup.cfg +++ b/metaflow/plugins/kfp/tests/setup.cfg @@ -13,7 +13,7 @@ exclude_lines = # pytest [tool:pytest] -addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html +addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug [html] directory = /home/zservice/public/coverage From da89fc3cef1035146f8e03ebaa8dbdfed44b5f28 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 09:47:54 -0700 Subject: [PATCH 07/17] AIP-6643: Turn back on log capturing, output to CLI and file descriptor separately --- .gitlab-ci.yml | 2 +- metaflow/plugins/kfp/tests/setup.cfg | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 97fe4edcdb8..0988e153205 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -72,7 +72,7 @@ stages: ${BUILT_IMAGE_FULL_PATH} bash -c " cd /home/zservice/metaflow/metaflow/plugins/kfp/tests && - python -m pytest -s -n 3 run_integration_tests.py --image ${BUILT_IMAGE_FULL_PATH} --opsgenie-api-token ${OPSGENIE_API_TOKEN} --cov-config=setup.cfg --public-dir /home/zservice/public + python -m pytest -n 3 run_integration_tests.py --image ${BUILT_IMAGE_FULL_PATH} --opsgenie-api-token ${OPSGENIE_API_TOKEN} --cov-config=setup.cfg --public-dir /home/zservice/public " artifacts: when: always diff --git a/metaflow/plugins/kfp/tests/setup.cfg b/metaflow/plugins/kfp/tests/setup.cfg index d2bc96bbb86..7977d22079b 100644 --- a/metaflow/plugins/kfp/tests/setup.cfg +++ b/metaflow/plugins/kfp/tests/setup.cfg @@ -13,7 +13,7 @@ exclude_lines = # pytest [tool:pytest] -addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug +addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug --log-cli-level info [html] directory = /home/zservice/public/coverage From 488d67e6787fcf23848612127ea710fc3461fffc Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 16:48:34 -0700 Subject: [PATCH 08/17] AIP-6643: Ensure we continue to capture stdout and stderr logs --- .gitlab-ci.yml | 2 +- metaflow/plugins/kfp/tests/conftest.py | 31 ++++++++++++++++++++++++-- metaflow/plugins/kfp/tests/setup.cfg | 3 ++- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 0988e153205..97fe4edcdb8 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -72,7 +72,7 @@ stages: ${BUILT_IMAGE_FULL_PATH} bash -c " cd /home/zservice/metaflow/metaflow/plugins/kfp/tests && - python -m pytest -n 3 run_integration_tests.py --image ${BUILT_IMAGE_FULL_PATH} --opsgenie-api-token ${OPSGENIE_API_TOKEN} --cov-config=setup.cfg --public-dir /home/zservice/public + python -m pytest -s -n 3 run_integration_tests.py --image ${BUILT_IMAGE_FULL_PATH} --opsgenie-api-token ${OPSGENIE_API_TOKEN} --cov-config=setup.cfg --public-dir /home/zservice/public " artifacts: when: always diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index 1e456baf321..663a5bfd199 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -1,5 +1,12 @@ -import pytest +import logging +import sys from pathlib import Path +from io import TextIOBase +import pytest + + +# Use the root logger for stdout/stderr patching. +logger = logging.getLogger() def pytest_addoption(parser): @@ -14,6 +21,22 @@ def pytest_addoption(parser): parser.addoption("--public-dir", dest="public_dir", action="store", default='') +class StreamToLogger(TextIOBase): + """Fake file-like stream object that redirects writes to a logger instance.""" + + def __init__(self, logger, level): + self.logger = logger + self.level = level + self.linebuf = "" + + def write(self, buf): + for line in buf.rstrip().splitlines(): + self.logger.log(self.level, line.rstrip()) + + def flush(self): + pass + + @pytest.hookimpl(hookwrapper=True, tryfirst=True) def pytest_runtest_setup(item): """Emit a log file per test to make it easier to debug in failure scenarios. @@ -27,4 +50,8 @@ def pytest_runtest_setup(item): f"{item._request.node.name}.log" ) logging_plugin.set_log_path(str(filename)) - yield + + # Forward logs from stdout/stderr to the logger as well. + sys.stdout = StreamToLogger(logger, logging.INFO) + sys.stderr = StreamToLogger(logger, logging.ERROR) + yield \ No newline at end of file diff --git a/metaflow/plugins/kfp/tests/setup.cfg b/metaflow/plugins/kfp/tests/setup.cfg index 7977d22079b..c86478fd191 100644 --- a/metaflow/plugins/kfp/tests/setup.cfg +++ b/metaflow/plugins/kfp/tests/setup.cfg @@ -13,7 +13,8 @@ exclude_lines = # pytest [tool:pytest] -addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug --log-cli-level info +addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug --log-cli-level info --logformat "%(asctime)s %(threadName)s %(filename)s:%(lineno)d %(levelname)s - %(message)s" --log-date-format "%Y-%m-%d %H:%M:%S" + [html] directory = /home/zservice/public/coverage From fc0ae1b05e18cc5f684994e76c023e3f9c580410 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Thu, 15 Sep 2022 16:55:09 -0700 Subject: [PATCH 09/17] AIP-6643: Ensure we continue to capture stdout and stderr logs --- metaflow/plugins/kfp/tests/setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metaflow/plugins/kfp/tests/setup.cfg b/metaflow/plugins/kfp/tests/setup.cfg index c86478fd191..2b1a65d3c68 100644 --- a/metaflow/plugins/kfp/tests/setup.cfg +++ b/metaflow/plugins/kfp/tests/setup.cfg @@ -13,7 +13,7 @@ exclude_lines = # pytest [tool:pytest] -addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug --log-cli-level info --logformat "%(asctime)s %(threadName)s %(filename)s:%(lineno)d %(levelname)s - %(message)s" --log-date-format "%Y-%m-%d %H:%M:%S" +addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug --log-cli-level info --log-format "%(asctime)s %(threadName)s %(filename)s:%(lineno)d %(levelname)s - %(message)s" --log-date-format "%Y-%m-%d %H:%M:%S" [html] From f4ba93f8479f9d8c8f24e5e25857a0726bf7eacc Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 20 Sep 2022 14:35:52 -0700 Subject: [PATCH 10/17] AIP-6643: Ensure stdout/stderr logs are both written to a file and proxied to the original streams. Additionally switch to just executing the foreach tests to delve into the 403 root cause --- metaflow/plugins/kfp/tests/conftest.py | 18 +++++++-------- .../kfp/tests/run_integration_tests.py | 22 ++++++++++++------- metaflow/plugins/kfp/tests/setup.cfg | 2 +- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index 663a5bfd199..674bf83ce48 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -24,14 +24,18 @@ def pytest_addoption(parser): class StreamToLogger(TextIOBase): """Fake file-like stream object that redirects writes to a logger instance.""" - def __init__(self, logger, level): + def __init__(self, logger, level, original_stream): self.logger = logger self.level = level self.linebuf = "" + self.original_stream = original_stream def write(self, buf): for line in buf.rstrip().splitlines(): self.logger.log(self.level, line.rstrip()) + else: + # Additionally ensure we also write back to the original stream too! + self.original_stream.write(buf) def flush(self): pass @@ -44,14 +48,10 @@ def pytest_runtest_setup(item): Sourced from: https://stackoverflow.com/a/64480499 """ logging_plugin = item.config.pluginmanager.get_plugin("logging-plugin") - filename = Path( - item.config.getoption('public_dir'), - 'pytest-logs', - f"{item._request.node.name}.log" - ) + filename = Path("pytest-logs", f"{item._request.node.name}.log") logging_plugin.set_log_path(str(filename)) # Forward logs from stdout/stderr to the logger as well. - sys.stdout = StreamToLogger(logger, logging.INFO) - sys.stderr = StreamToLogger(logger, logging.ERROR) - yield \ No newline at end of file + sys.stdout = StreamToLogger(logger, logging.INFO, sys.stdout) + sys.stderr = StreamToLogger(logger, logging.INFO, sys.stderr) + yield diff --git a/metaflow/plugins/kfp/tests/run_integration_tests.py b/metaflow/plugins/kfp/tests/run_integration_tests.py index 65b69902c85..81c7c823808 100644 --- a/metaflow/plugins/kfp/tests/run_integration_tests.py +++ b/metaflow/plugins/kfp/tests/run_integration_tests.py @@ -58,16 +58,23 @@ def _python(): def obtain_flow_file_paths(flow_dir_path: str) -> List[str]: - file_paths: List[str] = [ - file_name - for file_name in listdir(flow_dir_path) - if isfile(join(flow_dir_path, file_name)) - and not file_name.startswith(".") - and not file_name in non_standard_test_flows + # TODO AIP-6643 Reinstate all the tests before merging!!! + return [ + 'foreach_linear_foreach.py', + 'foreach_linear_split.py', + 'foreach_split_linear.py', ] - return file_paths + # file_paths: List[str] = [ + # file_name + # for file_name in listdir(flow_dir_path) + # if isfile(join(flow_dir_path, file_name)) + # and not file_name.startswith(".") + # and not file_name in non_standard_test_flows + # ] + # return file_paths +@pytest.mark.skip def test_s3_sensor_flow(pytestconfig) -> None: # ensure the s3_sensor waits for some time before the key exists file_name: str = f"s3-sensor-file-{uuid.uuid1()}.txt" @@ -205,7 +212,6 @@ def test_error_and_opsgenie_alert(pytestconfig) -> None: return -@pytest.mark.skip @pytest.mark.parametrize("flow_file_path", obtain_flow_file_paths("flows")) def test_flows(pytestconfig, flow_file_path: str) -> None: full_path: str = join("flows", flow_file_path) diff --git a/metaflow/plugins/kfp/tests/setup.cfg b/metaflow/plugins/kfp/tests/setup.cfg index 2b1a65d3c68..3f0f17e2ccb 100644 --- a/metaflow/plugins/kfp/tests/setup.cfg +++ b/metaflow/plugins/kfp/tests/setup.cfg @@ -13,7 +13,7 @@ exclude_lines = # pytest [tool:pytest] -addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug --log-cli-level info --log-format "%(asctime)s %(threadName)s %(filename)s:%(lineno)d %(levelname)s - %(message)s" --log-date-format "%Y-%m-%d %H:%M:%S" +addopts = -vv --cov=/home/zservice/metaflow/metaflow/plugins/kfp --cov-report term --cov-report html --log-file-level debug --log-cli-level info --log-file-format "%(asctime)s %(threadName)s %(filename)s:%(lineno)d %(levelname)s - %(message)s" --log-file-date-format "%Y-%m-%d %H:%M:%S" [html] From 02e3293af5cd88d6a4ed549c28190757b82c82af Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 20 Sep 2022 14:49:26 -0700 Subject: [PATCH 11/17] AIP-6643: Fix pytest-logs path --- metaflow/plugins/kfp/tests/conftest.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index 674bf83ce48..5b3580f16d0 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -48,7 +48,11 @@ def pytest_runtest_setup(item): Sourced from: https://stackoverflow.com/a/64480499 """ logging_plugin = item.config.pluginmanager.get_plugin("logging-plugin") - filename = Path("pytest-logs", f"{item._request.node.name}.log") + filename = Path( + item.config.getoption('public_dir'), + "pytest-logs", + f"{item._request.node.name}.log" + ) logging_plugin.set_log_path(str(filename)) # Forward logs from stdout/stderr to the logger as well. From fcc64d54158e4a6fbb3f3527421470344ed70a25 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 20 Sep 2022 15:16:12 -0700 Subject: [PATCH 12/17] AIP-6643: Reenable all the integration tests --- .../kfp/tests/run_integration_tests.py | 24 ++++++------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/metaflow/plugins/kfp/tests/run_integration_tests.py b/metaflow/plugins/kfp/tests/run_integration_tests.py index 81c7c823808..e06cd5fe60f 100644 --- a/metaflow/plugins/kfp/tests/run_integration_tests.py +++ b/metaflow/plugins/kfp/tests/run_integration_tests.py @@ -58,23 +58,16 @@ def _python(): def obtain_flow_file_paths(flow_dir_path: str) -> List[str]: - # TODO AIP-6643 Reinstate all the tests before merging!!! - return [ - 'foreach_linear_foreach.py', - 'foreach_linear_split.py', - 'foreach_split_linear.py', + file_paths: List[str] = [ + file_name + for file_name in listdir(flow_dir_path) + if isfile(join(flow_dir_path, file_name)) + and not file_name.startswith(".") + and not file_name in non_standard_test_flows ] - # file_paths: List[str] = [ - # file_name - # for file_name in listdir(flow_dir_path) - # if isfile(join(flow_dir_path, file_name)) - # and not file_name.startswith(".") - # and not file_name in non_standard_test_flows - # ] - # return file_paths + return file_paths -@pytest.mark.skip def test_s3_sensor_flow(pytestconfig) -> None: # ensure the s3_sensor waits for some time before the key exists file_name: str = f"s3-sensor-file-{uuid.uuid1()}.txt" @@ -138,7 +131,6 @@ def test_s3_sensor_flow(pytestconfig) -> None: # This test ensures that a flow fails correctly, # and when it fails, an OpsGenie email is sent. -@pytest.mark.skip def test_error_and_opsgenie_alert(pytestconfig) -> None: raise_error_flow_cmd: str = ( f"{_python()} flows/raise_error_flow.py --datastore=s3 kfp run " @@ -324,7 +316,6 @@ def get_compiled_yaml(compile_to_yaml_cmd, yaml_file_path) -> Dict[str, str]: return flow_yaml -@pytest.mark.skip def test_kubernetes_service_account_compile_only() -> None: service_account = "test-service-account" with tempfile.TemporaryDirectory() as yaml_tmp_dir: @@ -341,7 +332,6 @@ def test_kubernetes_service_account_compile_only() -> None: assert flow_yaml["spec"]["serviceAccountName"] == service_account -@pytest.mark.skip def test_toleration_and_affinity_compile_only() -> None: step_templates: Dict[str, str] = {} with tempfile.TemporaryDirectory() as yaml_tmp_dir: From 3d834d360d4c0a87096661871ee65c4da65d8317 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 20 Sep 2022 15:44:45 -0700 Subject: [PATCH 13/17] AIP-6643: Attempt to fix duplicate logs being written to file --- metaflow/plugins/kfp/tests/conftest.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index 5b3580f16d0..813c5ebe1c9 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -56,6 +56,8 @@ def pytest_runtest_setup(item): logging_plugin.set_log_path(str(filename)) # Forward logs from stdout/stderr to the logger as well. - sys.stdout = StreamToLogger(logger, logging.INFO, sys.stdout) - sys.stderr = StreamToLogger(logger, logging.INFO, sys.stderr) + if not isinstance(sys.stdout, StreamToLogger): + sys.stdout = StreamToLogger(logger, logging.INFO, sys.stdout) + if not isinstance(sys.stderr, StreamToLogger): + sys.stderr = StreamToLogger(logger, logging.INFO, sys.stderr) yield From c0ef94372b34ede0697560930b7d0ffff75c04ad Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 20 Sep 2022 16:29:10 -0700 Subject: [PATCH 14/17] AIP-6643: Fix pre-commit test --- metaflow/plugins/kfp/tests/conftest.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/metaflow/plugins/kfp/tests/conftest.py b/metaflow/plugins/kfp/tests/conftest.py index 813c5ebe1c9..a10e08075d7 100644 --- a/metaflow/plugins/kfp/tests/conftest.py +++ b/metaflow/plugins/kfp/tests/conftest.py @@ -18,7 +18,7 @@ def pytest_addoption(parser): parser.addoption( "--opsgenie-api-token", dest="opsgenie_api_token", action="store", default=None ) - parser.addoption("--public-dir", dest="public_dir", action="store", default='') + parser.addoption("--public-dir", dest="public_dir", action="store", default="") class StreamToLogger(TextIOBase): @@ -49,9 +49,9 @@ def pytest_runtest_setup(item): """ logging_plugin = item.config.pluginmanager.get_plugin("logging-plugin") filename = Path( - item.config.getoption('public_dir'), + item.config.getoption("public_dir"), "pytest-logs", - f"{item._request.node.name}.log" + f"{item._request.node.name}.log", ) logging_plugin.set_log_path(str(filename)) @@ -59,5 +59,5 @@ def pytest_runtest_setup(item): if not isinstance(sys.stdout, StreamToLogger): sys.stdout = StreamToLogger(logger, logging.INFO, sys.stdout) if not isinstance(sys.stderr, StreamToLogger): - sys.stderr = StreamToLogger(logger, logging.INFO, sys.stderr) + sys.stderr = StreamToLogger(logger, logging.INFO, sys.stderr) yield From b4db9ca145815e9637b973a5b74e89c3b513de7b Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 20 Sep 2022 19:10:58 -0700 Subject: [PATCH 15/17] AIP-6643: Limit the backoff retries so we can get logs upstream --- metaflow/plugins/kfp/tests/run_integration_tests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/metaflow/plugins/kfp/tests/run_integration_tests.py b/metaflow/plugins/kfp/tests/run_integration_tests.py index e06cd5fe60f..ef9e5b434b7 100644 --- a/metaflow/plugins/kfp/tests/run_integration_tests.py +++ b/metaflow/plugins/kfp/tests/run_integration_tests.py @@ -232,7 +232,8 @@ def run_cmd_with_backoff_from_platform_errors( # as well as output to stdout and stderr (which users can see on the Gitlab logs). We check # if the error message is due to a KFAM issue, and if so, we do an exponential backoff. - backoff_intervals_in_seconds: List[int] = [0, 2, 4, 8, 16, 32] + # TODO AIP-6643 Limit retries so we get all the logs + backoff_intervals_in_seconds: List[int] = [0, 2, 4] # , 8, 16, 32] platform_error_messages: List[str] = [ "Reason: Unauthorized", From 9cc76c044406b7407034e21a173915845a0b1ab2 Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 20 Sep 2022 20:05:37 -0700 Subject: [PATCH 16/17] AIP-6643: Only run nestedforeach tests --- metaflow/plugins/kfp/tests/run_integration_tests.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/metaflow/plugins/kfp/tests/run_integration_tests.py b/metaflow/plugins/kfp/tests/run_integration_tests.py index ef9e5b434b7..f8d92d8c13d 100644 --- a/metaflow/plugins/kfp/tests/run_integration_tests.py +++ b/metaflow/plugins/kfp/tests/run_integration_tests.py @@ -58,6 +58,8 @@ def _python(): def obtain_flow_file_paths(flow_dir_path: str) -> List[str]: + # TODO AIP-6643 Revert this + return ['nested_foreach_with_branching.py'] file_paths: List[str] = [ file_name for file_name in listdir(flow_dir_path) @@ -68,6 +70,7 @@ def obtain_flow_file_paths(flow_dir_path: str) -> List[str]: return file_paths +@pytest.mark.skip def test_s3_sensor_flow(pytestconfig) -> None: # ensure the s3_sensor waits for some time before the key exists file_name: str = f"s3-sensor-file-{uuid.uuid1()}.txt" @@ -131,6 +134,7 @@ def test_s3_sensor_flow(pytestconfig) -> None: # This test ensures that a flow fails correctly, # and when it fails, an OpsGenie email is sent. +@pytest.mark.skip def test_error_and_opsgenie_alert(pytestconfig) -> None: raise_error_flow_cmd: str = ( f"{_python()} flows/raise_error_flow.py --datastore=s3 kfp run " @@ -317,6 +321,7 @@ def get_compiled_yaml(compile_to_yaml_cmd, yaml_file_path) -> Dict[str, str]: return flow_yaml +@pytest.mark.skip def test_kubernetes_service_account_compile_only() -> None: service_account = "test-service-account" with tempfile.TemporaryDirectory() as yaml_tmp_dir: @@ -333,6 +338,7 @@ def test_kubernetes_service_account_compile_only() -> None: assert flow_yaml["spec"]["serviceAccountName"] == service_account +@pytest.mark.skip def test_toleration_and_affinity_compile_only() -> None: step_templates: Dict[str, str] = {} with tempfile.TemporaryDirectory() as yaml_tmp_dir: From c39b07a96f9b3fd21d512264faf1efe353d6564f Mon Sep 17 00:00:00 2001 From: Alex Latchford Date: Tue, 27 Sep 2022 10:15:41 -0700 Subject: [PATCH 17/17] AIP-6643: Add in verbose option to make it easier to see all DEBUG logs --- metaflow/plugins/kfp/kfp_cli.py | 14 ++++++++++++++ .../plugins/kfp/tests/run_integration_tests.py | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/metaflow/plugins/kfp/kfp_cli.py b/metaflow/plugins/kfp/kfp_cli.py index 9d7e79f3c69..dfc89107446 100644 --- a/metaflow/plugins/kfp/kfp_cli.py +++ b/metaflow/plugins/kfp/kfp_cli.py @@ -1,6 +1,7 @@ import json import shutil import subprocess +import logging from metaflow import JSONType, current, decorators, parameters from metaflow._vendor import click @@ -225,6 +226,13 @@ def step_init(obj, run_id, step_name, passed_in_split_indexes, task_id): "If not set, METAFLOW_NOTIFY_ON_SUCCESS is used from Metaflow config or environment variable", show_default=True, ) +@click.option( + "--verbose/--no-verbose", + "verbose", + default=False, + help="Turns on debug logging.", + show_default=True, +) @click.pass_obj def run( obj, @@ -248,12 +256,18 @@ def run( notify_on_error=None, notify_on_success=None, argo_wait=False, + verbose=False, **kwargs, ): """ Analogous to step_functions_cli.py """ + if verbose: + # Turn on debugging for the root logger so in particular we can see logs emitted by the KFP + # SDK as that by default emits there. + logging.basicConfig(level=logging.DEBUG) + def _convert_value(param: parameters.Parameter): v = kwargs.get(param.name) return json.dumps(v) if param.kwargs.get("type") == JSONType else v diff --git a/metaflow/plugins/kfp/tests/run_integration_tests.py b/metaflow/plugins/kfp/tests/run_integration_tests.py index f8d92d8c13d..5212f6a01a1 100644 --- a/metaflow/plugins/kfp/tests/run_integration_tests.py +++ b/metaflow/plugins/kfp/tests/run_integration_tests.py @@ -214,7 +214,7 @@ def test_flows(pytestconfig, flow_file_path: str) -> None: test_cmd: str = ( f"{_python()} {full_path} --datastore=s3 --with retry kfp run " - f"--wait-for-completion --workflow-timeout 1800 " + f"--wait-for-completion --workflow-timeout 1800 --verbose " f"--max-parallelism 3 --experiment metaflow_test --tag test_t1 " f"--sys-tag test_sys_t1:sys_tag_value " )