Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor status #205

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor status #205

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7292240
refactor status command so its logic can be reused by other commands
SHuang-Broad Apr 29, 2022
ae1aa5c
address comments
SHuang-Broad Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 33 additions & 18 deletions src/cromshell/status/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,47 @@ def main(config, workflow_id):
# Check if Cromwell Server Backend works
http_utils.assert_can_communicate_with_server(config)

requested_status_json, workflow_status, updated_ret_val = query_status(
config, workflow_id
)
ret_val = updated_ret_val

# Display status to user:
line_string = requested_status_json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why rename this here? Lets just print the requested_status_json after replacement.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Plus It isn't good practice to use type suffix when naming variables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Push back a little: I didn't change this as it was in the original code. I used pycharm to refactor out a function.
Regardless, I'll update this PR with the requested changes, and move this to utils.

print(line_string.replace(",", ",\n"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its fine as is but would be nice to use pretty_print_json()


# Update config.submission_file:
io_utils.update_all_workflow_database_tsv(
workflow_database_path=config.submission_file_path,
workflow_id=workflow_id,
column_to_update="STATUS",
update_value=workflow_status,
)

return ret_val


def query_status(config, workflow_id: str) -> (str, str, int):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not now but later, would be good to add unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, I'll add unit test in this PR.

"""

:param config: object holding configurations
:param workflow_id: ID for workflow whose status is being requested
:param log_to_screen: whether to log to screen the status
:return: (json string of the status, status of the workflow, return value of the main command)
"""

ret_val = 0

# Request workflow status
request_out = requests.get(
f"{config.cromwell_api_workflow_id}/status",
timeout=config.requests_connect_timeout,
verify=config.requests_verify_certs,
)

requested_status_json = request_out.content.decode("utf-8")
workflow_status_description = json.loads(request_out.content)

# Hold our status string here
workflow_status = workflow_status_description["status"]

# Set return value based on workflow status
if (
workflow_status
Expand All @@ -53,7 +81,7 @@ def main(config, workflow_id):
):
ret_val = 1
log.display_logo(io_utils.dead_turtle)
elif workflow_status == "Running":
elif workflow_status == cromshellconfig.WorkflowStatuses.Running.value[0]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird. I feel like there must be a better solution but we don't have to find it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate? I'm hoping to do it correctly in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like there should be a less awkward way of matching against the enum values. We shouldn't have to decompose the list manually. Maybe we should overload the equality option in the enum to check all the list values so we don't have to do this awkward thing.

# Status claims this workflow is running fine, but we need to check to see
# if there are any failed sub-processes.
# To do this, we get the workflow metadata and search for any failures
Expand Down Expand Up @@ -91,20 +119,7 @@ def main(config, workflow_id):

else:
log.display_logo(io_utils.turtle)

# Display status to user:
line_string = requested_status_json
print(line_string.replace(",", ",\n"))

# Update config.submission_file:
io_utils.update_all_workflow_database_tsv(
workflow_database_path=config.submission_file_path,
workflow_id=workflow_id,
column_to_update="STATUS",
update_value=workflow_status,
)

return ret_val
return requested_status_json, workflow_status, ret_val


def workflow_failed(metadata: dict):
Expand Down