DM-8208 merge Improve error handling on external task failure, pr #233

tgoldina · web-flow · commit 4d8a64226b9f · 2016-11-18T13:19:15.000-08:00
DM 8208 Improve error handling on external task failure
diff --git a/docs/firefly-external-task-launcher.md b/docs/firefly-external-task-launcher.md
@@ -105,21 +105,20 @@ A sample javascript, which builds up on the examples below is in
 Create an image viewer and place it into the `<div>` id `plotHere`.
 
 ```html
-<div id="plotHere" style="width: 350px; height: 350px;"></div>
+<div id="imageHere" style="width: 350px; height: 350px;"></div>
 ```
 
 ```js
  function onFireflyLoaded() {
-    var iv2= firefly.makeImageViewer("plotHere");
-    iv2.plot({
-        "id" :"FileFromExternalTask",
-        "launcher" :"python",
-        "task" :"someImageTask",
-        "taskParams" : {"p1":1,"p2":2},
-        "Title" :"Example FITS Image'",
-        "ColorTable" :"16",
-        "RangeValues":firefly.serializeRangeValues("Sigma",-2,8,"Linear")
-    });
+    var req = {
+        id : 'FileFromExternalTask',
+        launcher : 'python',
+        task : 'someImageTask',
+        taskParams : {p1:1,p2:2},
+        Title : 'FITS from Python task',
+        ColorTable : 2
+    };
+    firefly.showImage('imageHere', req);
  }
 ```
 
@@ -135,12 +134,11 @@ The table is plotted in the `<div>` id `tableHere`.
 
 ```js
  function onFireflyLoaded() {
-     var tableData= { "processor" : "TableFromExternalTask",
-         "launcher" : "python",
-         "task" : "TestTask3",
-         "taskParams" : { "param1" : "first arg", "param2" : "second arg" }
-     }; 
-     firefly.showTable(tableData, "tableHere");
+    var tblReq = firefly.util.table.makeTblRequest('TableFromExternalTask', 'Table from Python task',
+        { launcher : 'python', task : 'TableTask', taskParams : {p1: 1, p2: 2} }, // search parameters
+        { pageSize: 15} // table options
+    );
+    firefly.showTable('tableHere', tblReq);
  }
 ```
 
@@ -157,21 +155,23 @@ In this example, we get the histogram data from an exernal task and feed them to
 
 ```js
  function onFireflyLoaded() {
-      var launcher = 'python';
-      var task = 'JsonTaskToGetHistogramData';
-      var taskParams = { 'numbins': bins };
-      firefly.getJsonFromTask(launcher, task, taskParams)
-           .then(
-                function (histdata) {
-                     firefly.showHistogram(
-                          {'descr' : 'Histogram data returned from python JSON task',
-                           'binColor' : '#3d3033',
-                           'height' : 350,
-                           'data': histdata}, 'chartHere');
-                }
-           ).catch(function (reason) {
-                     console.log('Error fetching JSON data from '+launcher+' task '+task+': '+reason);
-                   }
-           );
+    var launcher = 'python';
+    var task = 'JsonTaskToGetHistogramData';
+    var taskParams = {'numbins': 10};
+    firefly.getJsonFromTask(launcher, task, taskParams)
+        .then(function (histdata) {
+            console.log('Returned JSON: ' + JSON.stringify(histdata));
+            firefly.util.renderDOM("chartHere", firefly.ui.Histogram,
+                {
+                    desc: 'Histogram data from Python JSON task',
+                    binColor: '#3d3033',
+                    height: 350,
+                    data: histdata
+                });
+        })
+        .catch(function (reason) {
+            console.error('Error fetching JSON data from ' + launcher + ' task ' + task + ': ' + reason);
+            document.getElementById('chartHere').innerHTML = '<p style="color:red">'+reason+'</p>';
+        });
  }
 ```
diff --git a/src/firefly/html/demo/ffapi-highlevel-charttest.html b/src/firefly/html/demo/ffapi-highlevel-charttest.html
@@ -11,7 +11,7 @@
     <meta http-equiv="Pragma" content="no-cache">
     <meta http-equiv="Expires" content="0">
     <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
-    <title>Demo of Firefly Tools</title>
+    <title>Charts in Firefly</title>
 </head>
 
 <body>
@@ -25,7 +25,7 @@ <h2>
 
 <div id="histogramHere" style="width: 800px; height: 550px; border: solid 1px"></div>
 <br/><br/>
-<div id="chartHere" style="width: 800px; height: 550px; border: solid 1px;"></div>
+<div id="xyplotHere" style="width: 800px; height: 550px; border: solid 1px;"></div>
 
 <script type="text/javascript">
     {
@@ -89,12 +89,10 @@ <h2>
                 xCol: 'ra1',
                 yCol: 'dec1'
             };
-            firefly.showXYPlot('chartHere', chartParams);
-
-
+            firefly.showXYPlot('xyplotHere', chartParams);
 
         }
-   }
+    }
 
 
 </script>
@@ -103,3 +101,5 @@ <h2>
 <script  type="text/javascript" src="../firefly_loader.js"></script>
 
 
+</body>
+</html>
diff --git a/src/firefly/html/demo/ffapi-pylaunch-test.html b/src/firefly/html/demo/ffapi-pylaunch-test.html
@@ -0,0 +1,74 @@
+<!DOCTYPE html>
+
+<!--
+  ~ License information at https://github.com/Caltech-IPAC/firefly/blob/master/License.txt
+  -->
+
+<!--
+  This is a test for Python (external task) launcher.
+  Firefly supports getting JSON data, table, or image from an external task
+  -->
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <title>Testing getJSONFromTask</title>
+</head>
+<body>
+    <div id="chartHere"  style="display:inline-block; width: 800px; height: 350px;
+                                border: solid 1px;"></div>
+    <br><br>
+    <div id="tableHere"  style="display:inline-block; width: 800px; height: 350px;
+                                border: solid 1px;"></div>
+    <br><br>
+    <div id="imageHere" style="width: 350px; height: 350px;"></div>
+</body>
+
+<script type="text/javascript">
+    {
+        onFireflyLoaded = function (firefly) {
+
+            // json - show histogram, generated by firefly/python/SamplePythonLauncher.py
+            var launcher = 'python';
+            var task = 'JsonTaskToGetHistogramData';
+            var taskParams = {'numbins': 10};
+            firefly.getJsonFromTask(launcher, task, taskParams)
+                    .then(function (histdata) {
+                        console.log('Returned JSON: ' + JSON.stringify(histdata));
+                        firefly.util.renderDOM("chartHere", firefly.ui.Histogram,
+                                {
+                                    desc: 'Histogram data returned from Python JSON task',
+                                    binColor: '#3d3033',
+                                    height: 350,
+                                    data: histdata
+                                });
+                    })
+                    .catch(function (reason) {
+                        console.error('Error fetching JSON data from ' + launcher + ' task ' + task + ': ' + reason);
+                        document.getElementById('chartHere').innerHTML = '<p style="color:red">'+reason+'</p>';
+                    });
+
+            // table - show table, generated by firefly/python/SamplePythonLauncher.py
+            var tblReq = firefly.util.table.makeTblRequest('TableFromExternalTask', 'Table from Python task',
+                    { launcher : 'python', task : 'TableTask', taskParams : {p1: 1, p2: 2} }, // search parameters
+                    { pageSize: 15} // table options
+            );
+            firefly.showTable('tableHere', tblReq);
+
+            // image - show FITS image, generated by firefly/python/SamplePythonLauncher.py
+            var req = {
+                id : 'FileFromExternalTask',
+                launcher : 'python',
+                task : 'someImageTask',
+                taskParams : {p1:1,p2:2},
+                Title : 'FITS from Python task',
+                ColorTable : 2
+            };
+            firefly.showImage('imageHere', req);
+        }
+    }
+</script>
+
+<script  type="text/javascript" src="../firefly_loader.js"></script>
+
+
+</html>
diff --git a/src/firefly/java/edu/caltech/ipac/firefly/server/ExternalTaskHandlerImpl.java b/src/firefly/java/edu/caltech/ipac/firefly/server/ExternalTaskHandlerImpl.java
@@ -75,13 +75,15 @@ public void setup(ExternalTaskLauncher launcher, Map<String, String> env) throws
             launcher.addParam("-o", ServerContext.getExternalPermWorkDir());
 
         } catch (Exception e) {
+            String err = null;
             if (e instanceof ParseException) {
                 ParseException pe = (ParseException)e;
-                LOGGER.error(e, "invalid JSON in taskParams; position: " + pe.getPosition() + "; " + pe.getMessage() + "; " +taskParams);
+                err = "Invalid JSON in taskParams: " + pe.toString() + " " +taskParams;
+                LOGGER.error(e, err);
             } else {
                 LOGGER.error(e);
             }
-            throw new InterruptedException(task+" launcher setup failed: "+e.getMessage());
+            throw new InterruptedException(task+" launcher setup failed: " + (err==null?e.getMessage():err));
         }
     }
 
@@ -146,7 +148,16 @@ public void handleOut(InputStream is) throws InterruptedException {
                             result = status.toString();
                         }
                     }
+                } else {
+                    setErrorIfEmpty("Unable to parse task status.");
                 }
+            } else {
+                if (collectStatus) {
+                    setErrorIfEmpty("No lines after "+STATUS_KEYWORD+".");
+                } else {
+                    setErrorIfEmpty("No "+STATUS_KEYWORD+" in external task standard output.");
+                }
+
             }
 
         }
@@ -158,6 +169,7 @@ public void handleError(InputStream is) throws InterruptedException {
         try {
             for (String line = reader.readLine(); (line != null); line = reader.readLine()) {
                 LOGGER.warn(line);
+                addError(line);
             }
         } catch (Exception e) {
             Logger.getLogger().error(e);
@@ -186,7 +198,7 @@ public File getOutfile() throws DataAccessException {
             throw new DataAccessException("Failed to obtain data. " + getError());
         } else {
             if (outfile == null) {
-                throw new DataAccessException("Output file is not available");
+                throw new DataAccessException("Output file is not returned from the task.");
             } else {
                 File ofile = new File(outfile);
                 if (!ofile.canRead()) {
@@ -211,6 +223,21 @@ public String getResult() throws DataAccessException {
 //============================================================================
 
 
+    private void setErrorIfEmpty(String error) {
+        if (this.error == null || this.error.trim().length() == 0) {
+            this.error = error;
+        }
+    }
+
+    private void addError(String error) {
+        if (this.error == null || this.error.trim().length() == 0) {
+            this.error = "";
+        } else {
+            this.error += " ";
+        }
+        this.error += error;
+    }
+
     private static java.nio.file.Path createWorkDir(String task) throws IOException {
         return Files.createTempDirectory(ServerContext.getExternalTempWorkDir().toPath(), task);
     }
diff --git a/src/firefly/java/edu/caltech/ipac/firefly/server/query/JsonFromExternalTask.java b/src/firefly/java/edu/caltech/ipac/firefly/server/query/JsonFromExternalTask.java
@@ -35,7 +35,7 @@ public String getUniqueID(ServerRequest request) {
         for (String p : ExternalTaskHandler.ALL_PARAMS) {
             String v = request.getParam(p);
             if (v != null) {
-                uid += "|" + p;
+                uid += "|" + v;
             }
         }
         return uid;
@@ -44,6 +44,9 @@ public String getUniqueID(ServerRequest request) {
     @Override
     public String getData(ServerRequest request) throws DataAccessException {
         String launcher = request.getParam(ExternalTaskHandler.LAUNCHER);
+        if (launcher == null) {
+            throw new DataAccessException(ExternalTaskHandler.LAUNCHER+" parameter is not found in request.");
+        }
         ExternalTaskLauncher taskLauncher = new ExternalTaskLauncher(launcher);
 
         try {
@@ -59,13 +62,13 @@ public String getData(ServerRequest request) throws DataAccessException {
             // get result from outfile
 
             if (!ServerContext.isFileInPath(outFile)) {
-                throw new SecurityException("Access is not permitted.");
+                throw new SecurityException("Access to "+outFile.getAbsolutePath()+" is not permitted.");
             }
 
             return FileUtil.readFile(outFile);
         } catch (Exception e) {
-            LOGGER.error(e, "Unable to data from external task: "+request.toString());
-            throw new DataAccessException("Unable to data from external task: "+e.getMessage());
+            LOGGER.error(e, "Unable get to data from external task: "+request.toString());
+            throw new DataAccessException("Unable to get data from external task: "+e.getMessage());
         }
     }
 
diff --git a/src/firefly/java/edu/caltech/ipac/firefly/server/query/SearchManager.java b/src/firefly/java/edu/caltech/ipac/firefly/server/query/SearchManager.java
@@ -16,10 +16,12 @@
 import edu.caltech.ipac.firefly.server.packagedata.PackageMaster;
 import edu.caltech.ipac.firefly.server.util.Logger;
 import edu.caltech.ipac.firefly.server.util.QueryUtil;
-import edu.caltech.ipac.firefly.server.util.ipactable.*;
+import edu.caltech.ipac.firefly.server.util.ipactable.DataGroupPart;
+import edu.caltech.ipac.firefly.server.util.ipactable.DataGroupReader;
+import edu.caltech.ipac.firefly.server.util.ipactable.IpacTableParser;
+import edu.caltech.ipac.firefly.server.util.ipactable.TableDef;
 import edu.caltech.ipac.util.Assert;
 import edu.caltech.ipac.util.IpacTableUtil;
-import org.json.simple.JSONObject;
 import org.json.simple.JSONValue;
 import org.json.simple.parser.JSONParser;
 import org.json.simple.parser.ParseException;
@@ -67,8 +69,8 @@ public String getJSONData(ServerRequest request) throws DataAccessException {
                 return jsonText;
             }
             catch(ParseException pe){
-                LOGGER.error(processor.getUniqueID(request) + " return invalid JSON; position: " + pe.getPosition() + "; " + pe.getMessage() + "; " + jsonText);
-                throw new DataAccessException("Request failed: invalid JSON; position: " + pe.getPosition() + "; " + pe.getMessage());
+                LOGGER.error(processor.getUniqueID(request) + " Can not parse returned JSON: " + pe.toString() + "\n" + jsonText);
+                throw new DataAccessException(request.getRequestId()+" Can not parse returned JSON: " + pe.toString());
             }
         } else {
             throw new DataAccessException("Request fail inspection.  Operation aborted.");
diff --git a/src/firefly/js/core/JsonUtils.js b/src/firefly/js/core/JsonUtils.js
@@ -83,7 +83,7 @@ export const jsonRequest= function(baseUrl, cmd, paramList, doPost) {
                 return;
             }
             response.json().then( (result) => {
-                if (has(result,'0')) {
+                if (has(result,'0.success')) {
                     if (toBoolean(result[0].success)) {
                         resolve(result[0].data ? result[0].data : result[0]);
                     }
diff --git a/src/firefly/js/rpc/SearchServicesJson.js b/src/firefly/js/rpc/SearchServicesJson.js
@@ -136,7 +136,7 @@ export const getJsonData = function(request) {
     paramList.push({name:ServerParams.REQUEST, value: request.toString()});
 
     return doService(doJsonP(), ServerParams.JSON_DATA, paramList
-    ).then((data) => {return JSON.parse(data); });
+    ).then((data) => {return data; });
 };
 
 /**
@@ -253,8 +253,6 @@ export const getDownloadProgress= function(fileKey) {
 
 
 /**
- *
- * @param {array|string} ids - one id or an array of ids
  * @param {string} email
  * @return {Promise}
  */
@@ -273,7 +271,7 @@ export const setEmail= function(email) {
 /**
  *
  * @param {array|string} ids one id or an array of ids
- * @param {JobAttributes} job attribute
+ * @param {JobAttributes} attribute job attribute
  * @return {Promise}
  */
 export const setAttribute= function(ids, attribute) {
diff --git a/src/firefly/js/tables/TableUtil.js b/src/firefly/js/tables/TableUtil.js
@@ -31,7 +31,7 @@ const LSSTQueryPID = 'LSSTCataLogSearch';
  * @param {object} [params] the parameters to include with this request.
  * @param {TableRequest} [options] more options.  see TableRequest for details.
  * @returns {TableRequest}
- * @pubic
+ * @public
  * @func  makeTblRequest
  * @memberof firefly.util.table
  */
@@ -52,9 +52,9 @@ export function makeTblRequest(id, title, params={}, options={}) {
  * @param {string} [alt_source] use this if source does not exists.
  * @param {TableRequest} [options]  more options.  see TableRequest for details.
  * @returns {TableRequest}
- * @pubic
+ * @public
  * @func makeFileRequest
- * @memberof firefly.util.table
+ *  @memberof firefly.util.table
  */
 export function makeFileRequest(title, source, alt_source, options={}) {
     const id = 'IpacTableFromSource';

Original file line number	Diff line number	Diff line change
`@@ -83,7 +83,7 @@ export const jsonRequest= function(baseUrl, cmd, paramList, doPost) {`
`83`	`83`	`return;`
`84`	`84`	`}`
`85`	`85`	`response.json().then( (result) => {`
`86`		`- if (has(result,'0')) {`
	`86`	`+ if (has(result,'0.success')) {`
`87`	`87`	`if (toBoolean(result[0].success)) {`
`88`	`88`	`resolve(result[0].data ? result[0].data : result[0]);`
`89`	`89`	`}`