Add fetch-logs and enable Azure log fetching #259
Conversation
src/cromshell/logs/command.py
Outdated
|
|
||
| def get_task_level_outputs(config, expand_subworkflows, requested_status) -> dict: |
There was a problem hiding this comment.
Add reason why we're using Metadata api instead of Cromwells logs API as comment in code
| "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) |
There was a problem hiding this comment.
Accept 1 id at a time because the output of multiple ids might overwhelm user
| "backendLogs", | ||
| "backend", | ||
| "shardIndex", | ||
| "stderr", |
There was a problem hiding this comment.
"As of now cromwell on Azure does not provide backendlogs, hence retrieving stderr and stdout". add as a comment
There was a problem hiding this comment.
Add the above comment as inline comment
| 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): |
There was a problem hiding this comment.
put json.dumps into a string variable instead of calling 3 times
|
Why aren't we using log API? |
| * 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. |
There was a problem hiding this comment.
indentations seem to be off?
There was a problem hiding this comment.
should be correct.
- command
- command description
- command description
| @@ -230,6 +234,173 @@ def copy_files_to_directory( | |||
| shutil.copy(inputs, directory) | |||
|
|
|||
|
|
|||
| def cat_file(file_path: str or Path, backend: str = None) -> str: | |||
There was a problem hiding this comment.
Not saying that you should do it here, but backend is a good candidate to be a enum
| else: | ||
| LOGGER.error( | ||
| "Caught an error while trying to download the file from Azure: %s", e | ||
| ) |
There was a problem hiding this comment.
do you want to re-raise the exception here?
Currently, it only emits an error message, but the program continues.
| return config.cromshell_config_options["azure_storage_account"] | ||
| except KeyError: | ||
| LOGGER.error( | ||
| "An 'azure_storage_account' is required for this action but" |
There was a problem hiding this comment.
In future PRs, we could support reading from env variables
| LOGGER.error( | ||
| "An 'azure_storage_account' is required for this action but" | ||
| "was not found in Cromshell configuration file. " | ||
| ) |
| @click.pass_obj | ||
| def main( | ||
| config, | ||
| workflow_id: str, | ||
| workflow_ids: list, |
There was a problem hiding this comment.
Let's switch it to supporting a single workflow.
src/cromshell/logs/command.py
Outdated
| 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'.", |
There was a problem hiding this comment.
Is status already an enum in config?
| or index.get("executionStatus") in requested_status | ||
| ): | ||
| all_task_logs[call].append( | ||
| { |
There was a problem hiding this comment.
Looks like a good candidate to be a class
|
|
||
| return did_print | ||
| check_for_empty_logs( |
There was a problem hiding this comment.
why is this necessary?
| print(f"{call}:") | ||
| for call_index in index_list: | ||
| if call_index is not None: | ||
| print_file_like_value_in_dict( |
There was a problem hiding this comment.
If we define a class like recommended above, this could be a method in that class.
…files that are empty
…to lack of default creds
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