Skip to content

DM 8208 Improve error handling on external task failure #233

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

Merged
merged 2 commits into from
Nov 18, 2016
Merged

DM 8208 Improve error handling on external task failure #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98ceafb
DM 8208 Improve error handling on external task failure
tgoldina Nov 17, 2016
e9be089
DM 8208
tgoldina Nov 18, 2016
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
66 changes: 33 additions & 33 deletions docs/firefly-external-task-launcher.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
```

Expand All @@ -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);
}
```

Expand All @@ -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>';
});
}
```
12 changes: 6 additions & 6 deletions src/firefly/html/demo/ffapi-highlevel-charttest.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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">
{
Expand Down Expand Up @@ -89,12 +89,10 @@ <h2>
xCol: 'ra1',
yCol: 'dec1'
};
firefly.showXYPlot('chartHere', chartParams);


firefly.showXYPlot('xyplotHere', chartParams);

}
}
}


</script>
Expand All @@ -103,3 +101,5 @@ <h2>
<script type="text/javascript" src="../firefly_loader.js"></script>


</body>
</html>
74 changes: 74 additions & 0 deletions src/firefly/html/demo/ffapi-pylaunch-test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
33 changes: 30 additions & 3 deletions src/firefly/java/edu/caltech/ipac/firefly/server/ExternalTaskHandlerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

Expand Down Expand Up @@ -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.");
}

}

}
Expand All @@ -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);
Expand Down Expand Up @@ -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()) {
Expand All @@ -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);
}
Expand Down
11 changes: 7 additions & 4 deletions src/firefly/java/edu/caltech/ipac/firefly/server/query/JsonFromExternalTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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());
}
}

Expand Down
10 changes: 6 additions & 4 deletions src/firefly/java/edu/caltech/ipac/firefly/server/query/SearchManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.");
Expand Down
2 changes: 1 addition & 1 deletion src/firefly/js/core/JsonUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may affect other cases where result has .error only. By adding this, it will not fall in to this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I just changed it back to how it used to be before 10/21.

As we talked today, this code is handling JSON wrapper of possibly non-JSON result. The wrapping is done in ServCommand processRequest:33 and 53 and the wrapper always has 'success' field set either to true or false. Hence, checking has(result,'0.success') is a way to tell if JSON result is wrapped.

The advantage of checking for 'success' field is that now we can pass arrays as JSON result.

Just a note: when we change how we wrap JSON result in ServCommand.java, we need to change jsonRequest in JsonUtil.js, namely this precise code.

if (toBoolean(result[0].success)) {
resolve(result[0].data ? result[0].data : result[0]);
}
Expand Down
6 changes: 2 additions & 4 deletions src/firefly/js/rpc/SearchServicesJson.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; });
};

/**
Expand Down Expand Up @@ -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}
*/
Expand All @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions src/firefly/js/tables/TableUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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';
Expand Down