Add fetch-logs and enable Azure log fetching by bshifaw · Pull Request #259 · broadinstitute/cromshell

bshifaw · 2023-05-09T14:47:43Z

Refracted Log command such that code is broken into smaller functions
Added option to download logs
Added Azure components such that files from azure storage containers can be read or downloaded

bshifaw · 2023-05-11T18:17:10Z

src/cromshell/logs/command.py

-                "a failure occurred before the workflow started."
-            )

+def get_task_level_outputs(config, expand_subworkflows, requested_status) -> dict:


Add reason why we're using Metadata api instead of Cromwells logs API as comment in code

bshifaw · 2023-05-11T18:19:22Z

src/cromshell/logs/command.py

-    "Separate multiple keys by comma or use 'ALL' to print all logs. "
-    "Some standard Cromwell status options are 'ALL', 'Done', 'RetryableFailure', 'Running', and 'Failed'.",
-)
+@click.argument("workflow_ids", required=True, nargs=-1)


Accept 1 id at a time because the output of multiple ids might overwhelm user

bshifaw · 2023-05-11T18:24:36Z

src/cromshell/logs/command.py

+        "backendLogs",
+        "backend",
+        "shardIndex",
+        "stderr",


"As of now cromwell on Azure does not provide backendlogs, hence retrieving stderr and stdout". add as a comment

Add the above comment as inline comment

bshifaw · 2023-05-11T18:30:43Z

src/cromshell/logs/command.py

+        LOGGER.error(f"No calls found for workflow: {workflow_id}")
+        raise Exception(f"No calls found for workflow: {workflow_id}")
+
+    if "log" not in json.dumps(workflow_logs):


put json.dumps into a string variable instead of calling 3 times

bshifaw · 2023-05-25T18:25:16Z

Why aren't we using log API?

SHuang-Broad

some minor issues

SHuang-Broad · 2023-05-25T17:48:10Z

README.md

-     * List the log files produced by a workflow.
-   * [COMING SOON] `fetch-logs [workflow-id] [[workflow-id]...]`              
-     * Download all logs produced by a workflow.
+     * List the log files produced by a workflow, Defaults to print `Failed` status only.


indentations seem to be off?

should be correct.

- command - command description - command description

SHuang-Broad · 2023-05-25T17:53:27Z

src/cromshell/utilities/io_utils.py

        shutil.copy(inputs, directory)


+def cat_file(file_path: str or Path, backend: str = None) -> str:


Not saying that you should do it here, but backend is a good candidate to be a enum

SHuang-Broad · 2023-05-25T17:57:06Z

src/cromshell/utilities/io_utils.py

+        else:
+            LOGGER.error(
+                "Caught an error while trying to download the file from Azure: %s", e
+            )


do you want to re-raise the exception here?
Currently, it only emits an error message, but the program continues.

SHuang-Broad · 2023-05-25T17:58:33Z

src/cromshell/utilities/io_utils.py

+        return config.cromshell_config_options["azure_storage_account"]
+    except KeyError:
+        LOGGER.error(
+            "An 'azure_storage_account' is required for this action but"


In future PRs, we could support reading from env variables

SHuang-Broad · 2023-05-25T17:58:52Z

src/cromshell/utilities/io_utils.py

+        LOGGER.error(
+            "An 'azure_storage_account' is required for this action but"
+            "was not found in Cromshell configuration file. "
+        )


SHuang-Broad · 2023-05-25T18:12:43Z

src/cromshell/logs/command.py

 def main(
    config,
-    workflow_id: str,
+    workflow_ids: list,


Let's switch it to supporting a single workflow.

SHuang-Broad · 2023-05-25T18:15:37Z

src/cromshell/logs/command.py

+    default="Failed",
+    help="Return a list with links to the task logs with the indicated status. "
+    "Separate multiple keys by comma or use 'ALL' to print all logs. "
+    "Some standard Cromwell status options are 'ALL', 'Done', 'RetryableFailure', 'Running', and 'Failed'.",


Is status already an enum in config?

SHuang-Broad · 2023-05-25T18:18:23Z

src/cromshell/logs/command.py

+                    or index.get("executionStatus") in requested_status
+                ):
+                    all_task_logs[call].append(
+                        {


Looks like a good candidate to be a class

SHuang-Broad · 2023-05-25T18:19:29Z

src/cromshell/logs/command.py

+                    )

-    return did_print
+    check_for_empty_logs(


why is this necessary?

SHuang-Broad · 2023-05-25T18:20:36Z

src/cromshell/logs/command.py

+        print(f"{call}:")
+        for call_index in index_list:
+            if call_index is not None:
+                print_file_like_value_in_dict(


If we define a class like recommended above, this could be a method in that class.

…tions

…s are not found

…files that are empty

…to lack of default creds

bshifaw added 6 commits April 28, 2023 10:03

added fetch wording to logs in README.md

09803c1

Merge branch 'main' into bs_fetch_logs

a53858b

first draft of logs refactoring

a2f86e3

Added 'azure_storage_account' to config options

1ec2a85

Add option to download from azure

aad6a58

update log readme options

88a5430

bshifaw self-assigned this May 9, 2023

minor edits, removed function that called logs api

b0df586

bshifaw added the Cromshell 2 Issues related to Cromshell 2.0 label May 9, 2023

bshifaw added this to the Cromshell2.1 milestone May 9, 2023

bshifaw added 2 commits May 9, 2023 14:26

lint fixes

88077ed

updated requirements.txt to include azure packages

a7b82fc

bshifaw changed the title ~~Bs fetch logs~~ Add fetch-logs and enable Azure log fetching May 10, 2023

bshifaw commented May 11, 2023

View reviewed changes

SHuang-Broad mentioned this pull request May 12, 2023

can't access workflow logs from AoU RWB #260

Closed

Minor updates to command.py, added several unit tests for logs command

09a3859

SHuang-Broad requested changes May 25, 2023

View reviewed changes

bshifaw added 9 commits June 26, 2023 18:31

Added function to replace uuids in integration tests

f7ee8cd

functional integration tests for logs

21bfa56

refractoring logs command.py

4cb1e6c

show default status to retrieve in help message

e1d6316

Allow the user to specify where to download logs if needed

f6e4977

made fetch_logs and printing log summary option mutually exclusive op…

b826399

…tions

linting changes

67b0566

linting changes, fix to path for test function

a79f0e1

Added more explicit message about using ALL when default 'Failed' log…

00fe4ed

…s are not found

bshifaw marked this pull request as ready for review June 28, 2023 18:33

bshifaw added 7 commits June 28, 2023 21:59

fix integration test

1228b5a

enum for backends

389eae3

Added reason why the logs api isn't being used by the logs command

69ac907

added unit tests for io utils functions, added warning for azure/gcp …

07fa6a2

…files that are empty

lint fix

5a6f3dc

removed get_gcp_file_content because it requires in git tox test due …

7a7459e

…to lack of default creds

lint fix

ff3ff45

		shutil.copy(inputs, directory)


		def cat_file(file_path: str or Path, backend: str = None) -> str:

Conversation

bshifaw commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshifaw May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bshifaw commented May 25, 2023

Uh oh!

SHuang-Broad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bshifaw commented May 9, 2023 •

edited

Loading

bshifaw May 11, 2023 •

edited

Loading