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

Detect and display non-zero returncode from scripts #474

Merged
merged 10 commits into from
Oct 19, 2023
Merged

Detect and display non-zero returncode from scripts #474

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
803f18c
Detect and display non-zero returncode from scripts
will-moore May 24, 2023
bd9f57d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 24, 2023
f5384a2
Add returncode to error message
will-moore May 24, 2023
f4c0fcd
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 24, 2023
1879956
flake8 fix
will-moore May 24, 2023
8081fa7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 24, 2023
c9a586d
Revert Comment change to avoid merge conflict
will-moore May 29, 2023
c88fdf7
Merge remote-tracking branch 'origin/master' into script_failure_hand…
will-moore May 31, 2023
659d4c5
Always use status:'finished' when done, even if error
will-moore Oct 10, 2023
760da8e
Use 'returncode' instead of 'failure' and 'error'
will-moore Oct 18, 2023
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
15 changes: 1 addition & 14 deletions omeroweb/webclient/templates/webclient/activities/activitiesContent.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,21 +241,8 @@

{% else %}
{% if j.error %}
<div class='script_error' title="{{ j.error }}">
<div class='script_error' title="Script failed. Please see error logs">
<img alt="Failed to run script properly" src="{% static "webgateway/img/failed.png" %}" />


{% comment %}
<!-- Don't allow submitting now. TODO: launch submit page -->
<img src="{% static "webclient/image/info16.png" %}"/>
<input type='submit' title="Send Error as Feeback to the OME team" jobKey="{{ j.key }}"
{% if j.error_sent %}
value='Thank you' disabled='true'
{% else %}
value='Submit Feedback'
{% endif %} />
{% endcomment %}

</div>
{% else %}
<img alt="Success" src="{% static "webgateway/img/success.png" %}" />
Expand Down
22 changes: 20 additions & 2 deletions omeroweb/webclient/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3524,6 +3524,13 @@ def update_callback(request, cbString, **kwargs):
request.session["callback"][cbString][key] = value


# Subclass to handle returncode
class ScriptsCallback(omero.scripts.ProcessCallbackI):
def processFinished(self, returncode, current=None):
super().processFinished(returncode, current)
self.returncode = returncode


@login_required()
@render_response()
def activities(request, conn=None, **kwargs):
Expand Down Expand Up @@ -3799,14 +3806,25 @@ def activities(request, conn=None, **kwargs):
error=1,
)
continue
cb = omero.scripts.ProcessCallbackI(conn.c, proc)
cb = ScriptsCallback(conn.c, proc)
# check if we get something back from the handle...
if cb.block(0): # ms.
cb.close()
try:
# we can only retrieve this ONCE - must save results
results = proc.getResults(0, conn.SERVICE_OPTS)
update_callback(request, cbString, status="finished")
kwargs = {
"status": ("finished" if cb.returncode == 0 else "failed"),
Copy link
Member

Choose a reason for hiding this comment

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

I initially thought that this key was reflecting the process termination state i.e. the enums defined in https://github.com/ome/omero-py/blob/d5ca72f930d85ee1f147f9f8e3cfd1c95c8089ae/src/omero/callbacks.py#L62-L64 but looking at the code, we seem to have additional state e.g. in-progress, failed etc

Do we have a list of all supported statuses with their meaning?
In that case, I suspect finished must be interpreted as finished with a zero return code. Note this is a more restrictive interpretation than https://docs.openmicroscopy.org/omero-blitz/5.6.2/slice2html/omero/grid/ProcessCallback.html#processFinished.

Copy link
Member Author

Choose a reason for hiding this comment

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

From testing in omero-figure, that app is using the "finished" status to know when the figure export script is done. Without it, the spinner keeps on spinning and the stderr isn't displayed.
So, best not to change this if we can help it.

"failure": cb.returncode,
}
if cb.returncode != 0:
# This 'error' shows failure icon
kwargs["error"] = 1
Copy link
Member

Choose a reason for hiding this comment

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

Reading the code, if return code is non-zero, there are currently three keys which have updated values (failure, status and error). Are these all necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

failure isn't used anywhere so I'll remove it.
The error flag is a boolean, so we could remove it and add set status = "error" for scripts.

kwargs["Message"] = (
f"Script exited with failure."
f" (returncode={ cb.returncode })"
)
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if a script first set a message output via client.setOutput and then terminate with a non-zero return code?
Should we handle this use case and respect the message sent by the script and use this as the default error is unset?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sbesson Just returning to this... In my testing, if the script does client.setOutput(....) then generates some un-caught exception and terminates, the output is not passed on to the server. I don't see it in the code at

omero-web/omeroweb/webclient/views.py

Line 3822 in d5e4f84

for key, value in results.items():

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies - made a mistake in my client.setOutput("Message"...). That is actually handled OK and is displayed in place of the error message:

Screenshot 2023-09-08 at 14 37 37

However, most scripts don't try to return a "Message" until the end, so it's likely that if a script fails with some uncaught exception then the "Message" won't have been set.

update_callback(request, cbString, **kwargs)
new_results.append(cbString)
except Exception:
update_callback(
Expand Down