-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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
Exit on follow by inspecting the current log line #207
Conversation
floyd/cli/experiment.py
Outdated
failure = "[failed] Task execution failed" | ||
shutdown = "[shutdown] Task execution cancelled" | ||
timeout = "[timeout] Task execution cancelled" | ||
termination_list = [success, failure, shutdown, timeout] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better move this list creation outside so we are not creating a new list on every function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, move this outside the function to the top of this file. Also since these are constants use capitalized variable names for easy identification.
SUCCESS_OUTPUT = "[success] Finished execution"
FAILURE_OUTPUT = "[failed] Task execution failed"
..
TERMINATION_OUTPUT_LIST = [SUCCESS_OUTPUT, FAILURE_OUTPUT, ..]
floyd/cli/experiment.py
Outdated
timeout = "[timeout] Task execution cancelled" | ||
termination_list = [success, failure, shutdown, timeout] | ||
|
||
if any(terminal_line in log_output_line for terminal_line in termination_list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for simplification, you can just write return any(terminal_line in log_output_line for terminal_line in termination_list)
.
floyd/cli/experiment.py
Outdated
# Get the logs in a loop and log the new lines | ||
log_file_contents = ResourceClient().get_content(instance_log_id) | ||
print_output = log_file_contents[cur_idx:] | ||
# Get the status of the Job from the current log line | ||
job_terminated = check_job_termination_line(print_output, termination_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simplified as:
job_terminated = print_output in termination_list
Then you can remove the extra function call here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot go for job_terminated = print_output in termination_list
, because it's conditioning for a perfect match while in this case, I'm performing a substring match.
floyd/cli/experiment.py
Outdated
failure = "[failed] Task execution failed" | ||
shutdown = "[shutdown] Task execution cancelled" | ||
timeout = "[timeout] Task execution cancelled" | ||
termination_list = [success, failure, shutdown, timeout] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, move this outside the function to the top of this file. Also since these are constants use capitalized variable names for easy identification.
SUCCESS_OUTPUT = "[success] Finished execution"
FAILURE_OUTPUT = "[failed] Task execution failed"
..
TERMINATION_OUTPUT_LIST = [SUCCESS_OUTPUT, FAILURE_OUTPUT, ..]
Ticket: When using --follow from CLI, command doesn't exit when job is done