-
Notifications
You must be signed in to change notification settings - Fork 32
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
Additions to the workflow-endpoint #1108
Conversation
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.
Okay, I thought of a few other small quality-of-life improvements. Since there is an endpoint for checking the workflow job status, potentially we could further improve:
|
To not generate too much output Co-authored-by: Mehmed Mustafa <[email protected]>
Co-authored-by: Mehmed Mustafa <[email protected]>
if failed_tasks_key not in res: | ||
res[failed_tasks_key] = failed_tasks | ||
failed_tasks.setdefault(job.processor_name, []) | ||
failed_tasks[job.processor_name].append(job.job_id) |
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 think we should append here more than just the job.job_id
. In my suggestion, it was the entire job
model but that may still fill the output with unnecessary details (bad). My idea there was that the user could already get all the information needed for the failed jobs (mainly the page_id
and potentially the log file path that will come in the future) instead of checking the details of each job with separate requests to the processing server again.
Although I am not sure what is the best here. It all comes down to what will be easier for the user when automatically parsing the response. Maybe @kba could provide more hints regarding that. We could also keep it as it is now. This is fine for the current state.
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.
Ok, I didn't realize that you wanted it that way. I think then it would be better if we e.g. I at least add the page_id for now so that it is easier to adapt later and that it is more obvious that more fields would potentially be available.
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 have now added the page_id. I agree that the logfile-path would be helpful. We should add that sometime when it is possible.
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.
LGTM. We should continue the discussion about what best to return to users in a separate issue after release.
This contains a new endpoint to query the status of a workflow run and improved error handling for the workflow-enpoint. This were some of the parts we realized in our last meeting we need improvements in.