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.

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
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@



<!-- Status -->
<!-- Scripts -->
{% ifequal j.job_type "script" %}
<tr id="{{ j.id }}" class="script{% if j.new %} new_result{% endif %}{% ifequal j.status 'in progress' %} in_progress{% endifequal %}">

Expand All @@ -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
19 changes: 17 additions & 2 deletions omeroweb/webclient/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3522,6 +3522,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 @@ -3797,14 +3804,22 @@ 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"] = "Script failed with Exception"
Copy link
Member

Choose a reason for hiding this comment

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

Should this not just be kwargs["error"] = ...

Also, I could see adding the return code into the message unobstruvisely, e.g.:

f"Script exited with failure. (rc={{ cb.returnCode }})"

Copy link
Member Author

Choose a reason for hiding this comment

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

It is kwargs["error"] = ... ?
Do you mean kwargs["error"] = cb.returncode ?

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