Conversation
| command_line: str, | ||
| mount_pairs: list[dict] | None = None, | ||
| name_suffix: str = "", | ||
| blob_container: str | None = None, |
There was a problem hiding this comment.
I'm not sure we need to introduce another parameter to this function. We're already saving the blob container name that is specified in save_logs_to_blob in create_job() and saved to self. This could get passed in directly when we need it.
| ) | ||
| if rel_mnt_path != "ERROR!": | ||
| rel_mnt_path = "/" + helpers.format_rel_path(rel_path=rel_mnt_path) | ||
| rel_mnt_path = "/" |
There was a problem hiding this comment.
do we still need this rel_mnt_path = "/" when we aren't appending the stdout/stderr file names to the task command?
| @@ -838,16 +840,7 @@ def add_task( | |||
| logger.debug(f"Using provided container name: {container_name}.") | |||
|
|
|||
| if self.save_logs_to_blob: | |||
There was a problem hiding this comment.
we could maybe get rid of this check on if self.save_logs_to_blob here and just create the OutputFile in the batch_helpers.add_task
| job_name=job_name, | ||
| task_id_base=job_name, | ||
| command_line=command_line, | ||
| save_logs_rel_path=rel_mnt_path, |
There was a problem hiding this comment.
do we use rel_mnt_path anywhere anymore? If not, we could change the save_logs_rel_path parameter name to something like save_log_blob_container that a user can input a blob container directly or if it's None here then we can pull from self.save_logs_to_blob. This can replace the blob_container parameter below since they both do the same thing
| logger.debug(f"Stdout will be saved to: '{stdout_file}'") | ||
| logger.debug(f"Stderr will be saved to: '{stderr_file}'") | ||
| return f"""/bin/bash -c "mkdir -p {_folder}; {command_line} > {stdout_file} 2> {stderr_file}" """ | ||
| return f"""/bin/bash -c "mkdir -p {_folder}; {command_line} > >(tee {stdout_file}) 2> >(tee {stderr_file})" """ |
There was a problem hiding this comment.
if we're using OutputFiles for saving logs to blob then we don't need to do any of the tee commands. We can use the command that the user provides
| @@ -1309,24 +1324,17 @@ def add_task( | |||
| logger.debug(f"Generated string-based task ID: '{task_id}'") | |||
|
|
|||
| if save_logs_rel_path is not None: | |||
There was a problem hiding this comment.
could this whole check be removed?
There was a problem hiding this comment.
We still need this check so I can modify the command sent to Azure task.
| working_directory="containerImageDefault", | ||
| ), | ||
| output_files = None | ||
| if blob_container and blob_storage_account: |
There was a problem hiding this comment.
instead of blob_container we check for save_logs_blob_container or whatever the parameter name is that you choose (from the comment above)
There was a problem hiding this comment.
CloudClient will pass the self.save_logs_blob_container as value of blob_container parameter to batch_helpers.add_task
| file_pattern="../std*.txt", | ||
| blob_container=blob_container, | ||
| blob_account=blob_storage_account, | ||
| path=job_name, |
There was a problem hiding this comment.
for the path we may want to save to job_name/task_id. I believe the output of each task is simply a stdout.txt and stderr.txt so we will need a way to distinguish between tasks too
| [project] | ||
| name = "cfa.cloudops" | ||
| version = "0.3.22" | ||
| version = "0.3.23" |
There was a problem hiding this comment.
since we're doing more changes and possibly renaming parameters, I would suggest bumping the version to 0.4.0 for the next minor update
|
when I test the code on my side to input-test with a logs folder of <job_name>_logs, I get stdout and stderr saved here with long title names, rather than the nested job_nam/task_name/stdout.txt that I expected. It makes me think the OutputFiles isn't working and it's the task output redirection doing the work. |
3 participants
No description provided.