Detect and display non-zero returncode from scripts #474

will-moore · 2023-05-24T11:41:35Z

This uses the returncode returned from running scripts to show the Error icon in Activities dialog:

Feature requested at https://forum.image.sc/t/omero-scripts-fail-icon/81495

To test:

Run a script that throws an Exception after setting message, e.g. client.setOutput("Message", rstring("OK so far..."))
- The Message will be shown, along with Error status (first result below)
Run a script that throws an Exception without setting "Message" (typical behaviour)
- This will display a message including the returncode from the failure.
Otherwise working scripts should behave as before (2 other examples below)

The context used to render the html page in the example above looks like this:

{
  "sizeOfJobs": 4,
  "jobs": [
    {
      "job_type": "script",
      "job_name": "Images From ROIs",
      "start_time": "datetime.datetime(2023, 10, 18, 10, 15, 20, 121058)",
      "status": "finished",
      "returncode": 1,
      "Message": "OK so far...",
      "stderr": 3725,
      "results": {},
      "id": "52dd4857-9ca1-4dd1-ad0b-5a78d3fdd235",
      "key": "ProcessCallback/52dd4857-9ca1-4dd1-ad0b-5a78d3fdd235 -t -e 1.1:tcp -h 172.19.0.4 -p 37387 -t 60000",
      "new": true
    },
    {
      "job_type": "script",
      "job_name": "Dataset To Plate",
      "start_time": "datetime.datetime(2023, 10, 18, 10, 16, 0, 665853)",
      "status": "finished",
      "returncode": 1,
      "Message": "Script exited with failure. (returncode=1)",
      "stderr": 3724,
      "results": {},
      "id": "087c20ed-0617-4931-9ef4-91046bd1181e",
      "key": "ProcessCallback/087c20ed-0617-4931-9ef4-91046bd1181e -t -e 1.1:tcp -h 172.19.0.4 -p 37387 -t 60000"
    },
    {
      "job_type": "script",
      "job_name": "Dataset To Plate",
      "start_time": "datetime.datetime(2023, 10, 18, 10, 15, 20, 121058)",
      "status": "finished",
      "returncode": 0,
      "Message": " New plate created: From_ROIs but could not be attached.",
      "results": {
        "New_Object": {
          "id": 101,
          "type": "Plate",
          "browse_url": "/webclient/userdata/?show=plate-101",
          "name": "From_ROIs"
        }
      },
      "id": "e31c309c-947c-4fa7-a8ff-1471f31a52d1",
      "key": "ProcessCallback/e31c309c-947c-4fa7-a8ff-1471f31a52d1 -t -e 1.1:tcp -h 172.19.0.4 -p 37387 -t 60000"
    },
    {
      "job_type": "script",
      "job_name": "Channel Offsets",
      "start_time": "datetime.datetime(2023, 10, 18, 10, 10, 54, 262510)",
      "status": "finished",
      "returncode": 0,
      "Message": "New image created: P-TRE_10_R3D_D3D.dv_0_offsets.",
      "stdout": 3719,
      "results": {
        "Image": {
          "id": 751,
          "type": "Image",
          "browse_url": "/webclient/userdata/?show=image-751",
          "name": "P-TRE_10_R3D_D3D.dv_0_offsets"
        }
      },
      "id": "6447e666-fc04-42ff-955c-d1cfe2e23035",
      "key": "ProcessCallback/6447e666-fc04-42ff-955c-d1cfe2e23035 -t -e 1.1:tcp -h 172.19.0.4 -p 37387 -t 60000"
    }
  ],
  "inprogress": 0,
  "new_results": 1,
  "new_errors": true,
  "failure": 0
}

for more information, see https://pre-commit.ci

imagesc-bot · 2023-05-24T11:41:55Z

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-scripts-fail-icon/81495/7

joshmoore · 2023-05-24T11:51:03Z

omeroweb/webclient/views.py

+                        if cb.returncode != 0:
+                            # This 'error' shows failure icon
+                            kwargs["error"] = 1
+                            kwargs["Message"] = "Script failed with Exception"


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 }})"

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

for more information, see https://pre-commit.ci

sbesson · 2023-05-24T16:15:19Z

Interesting thread which incidentally raises the question: what the recommendation should be for handling and communicating failures in OMERO.scripts?

At the moment, the primary relevant place I found in the reference documentation suggests failures may be handled via client output message - see https://omero.readthedocs.io/en/stable/developers/scripts/style-guide.html#script-outputs

Using the official Populate_Metadata.py OMERO script as an exmple, there is already a mixture of behaviours:

some failing checks write to sys.stderr and raisesys.exit(1) - e.g. https://github.com/ome/omero-scripts/blob/88219a3323685a575f92e689222b91b4e590da3c/omero/import_scripts/Populate_Metadata.py#L65-L68. These would be displayed with an error icon as per the current state of this PR
other locations return an error message which is sent back to the client as per the reference guide - see https://github.com/ome/omero-scripts/blob/88219a3323685a575f92e689222b91b4e590da3c/omero/import_scripts/Populate_Metadata.py#L112-L115

will-moore · 2023-05-31T15:15:43Z

@sbesson Thanks for the review. I guess we can't currently have the script raise an Exception (to get the failed icon) and also return a useful Error message. But the Exception will be captured in stdout - do we need to make it more obvious that the user should look there?

I would prefer not to add another convention (on top of the "Message" convention) for errors, so I think that raising an Exception should be the best way to indicate "Failure".

…ling

sbesson · 2023-06-06T07:25:03Z

omeroweb/webclient/views.py

                # 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"),


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.

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.

sbesson · 2023-06-06T07:26:27Z

omeroweb/webclient/views.py

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


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?

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.

sbesson · 2023-06-06T07:28:42Z

omeroweb/webclient/views.py

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


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?

@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():

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

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.

will-moore · 2023-10-11T13:26:13Z

That last commit fixed the display of Error reporting in Figure:

sbesson · 2023-10-17T21:06:56Z

@will-moore following up on the questions from #474 (comment), would it be possible to either update the description of this PR or add a comment with the different types of scenarios that should be tested and the expected outcome of the callback dictionary ?

will-moore · 2023-10-18T09:38:28Z

@sbesson I've updated the PR description. Hope that contains everything you need?

sbesson · 2023-10-18T10:00:43Z

Thanks @will-moore for adjusting the description. As per the previous discussion, I am still questioning the added value of the new failure key as its value seems to be completely redundant with the presence/value of error. I thought the plan was to remove it as per #474 (comment) ?

Also re #474 (comment), has the proposal of storing the value of the return code in error and adjust the HTML template to handle the non-zero scenario been discussed further?

will-moore · 2023-10-18T10:05:29Z

OK - yes, apologies. I'll do that...

will-moore · 2023-10-18T10:54:02Z

@sbesson Use key returncode now for the returncode! Updated the JSON in the description accordingly.

sbesson

Tested with a minimal script

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import omero.scripts as scripts
from omero.gateway import BlitzGateway
from omero.rtypes import rstring

def run_script():
    client = scripts.client(
        'test_failure.py',
        """Test""",
        scripts.Bool(
            "Fail", optional=False, grouping="1", default="False"),
        scripts.String(
            "Message", optional=True, grouping="2",
            description="The message to be sent to the client."),
    )

    try:
        conn = BlitzGateway(client_obj=client)
        script_params = client.getInputs(unwrap=True)
        
        if "Message" in script_params:
            client.setOutput("Message", rstring(script_params["Message"]))
        if script_params["Fail"]:
            raise Exception("This should fail the script")
    finally:
        client.closeSession()


if __name__ == "__main__":
    run_script()

After running the script with various conditions of failures/messages

The underlying changes should remain backwards-compatible for existing apps which would rely on the context and make the return code available.
Overall, communicating a non-zero return code is definitely useful and I think this change is safe for inclusion in the next minor release of OMERO.web.

will-moore and others added 2 commits May 24, 2023 12:16

Detect and display non-zero returncode from scripts

803f18c

[pre-commit.ci] auto fixes from pre-commit.com hooks

bd9f57d

for more information, see https://pre-commit.ci

joshmoore reviewed May 24, 2023

View reviewed changes

will-moore and others added 4 commits May 24, 2023 13:00

Add returncode to error message

f5384a2

[pre-commit.ci] auto fixes from pre-commit.com hooks

f4c0fcd

for more information, see https://pre-commit.ci

flake8 fix

1879956

[pre-commit.ci] auto fixes from pre-commit.com hooks

8081fa7

for more information, see https://pre-commit.ci

Revert Comment change to avoid merge conflict

c9a586d

ome deleted a comment from snoopycrimecop May 31, 2023

Merge remote-tracking branch 'origin/master' into script_failure_hand…

c88fdf7

…ling

sbesson reviewed Jun 6, 2023

View reviewed changes

Always use status:'finished' when done, even if error

659d4c5

knabar requested review from sbesson and joshmoore October 17, 2023 18:31

knabar added this to the 5.23.0 milestone Oct 17, 2023

Use 'returncode' instead of 'failure' and 'error'

760da8e

sbesson approved these changes Oct 19, 2023

View reviewed changes

knabar merged commit 3a5d88d into ome:master Oct 19, 2023
7 checks passed

will-moore mentioned this pull request Mar 7, 2024

Don't overwrite kwargs in activities json #541

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect and display non-zero returncode from scripts #474

Detect and display non-zero returncode from scripts #474

will-moore commented May 24, 2023 •

edited

Loading

imagesc-bot commented May 24, 2023

joshmoore May 24, 2023

will-moore May 24, 2023

sbesson commented May 24, 2023 •

edited

Loading

will-moore commented May 31, 2023

sbesson Jun 6, 2023

will-moore Oct 10, 2023

sbesson Jun 6, 2023

will-moore Sep 11, 2023

sbesson Jun 6, 2023

will-moore Sep 8, 2023

will-moore Sep 8, 2023

will-moore commented Oct 11, 2023

sbesson commented Oct 17, 2023

will-moore commented Oct 18, 2023

sbesson commented Oct 18, 2023

will-moore commented Oct 18, 2023

will-moore commented Oct 18, 2023

sbesson left a comment

Detect and display non-zero returncode from scripts #474

Detect and display non-zero returncode from scripts #474

Conversation

will-moore commented May 24, 2023 • edited Loading

imagesc-bot commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbesson commented May 24, 2023 • edited Loading

will-moore commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

will-moore commented Oct 11, 2023

sbesson commented Oct 17, 2023

will-moore commented Oct 18, 2023

sbesson commented Oct 18, 2023

will-moore commented Oct 18, 2023

will-moore commented Oct 18, 2023

sbesson left a comment

Choose a reason for hiding this comment

will-moore commented May 24, 2023 •

edited

Loading

sbesson commented May 24, 2023 •

edited

Loading