-
Notifications
You must be signed in to change notification settings - Fork 23
VIP input maps handling #658
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| import java.util.Map; | ||
| import java.util.Map.Entry; | ||
| import java.util.function.Supplier; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
@@ -174,18 +175,21 @@ private Execution getExecutionFromSimulation(Simulation s, boolean summarize) th | |
| for (InOutData iod : inputs) { | ||
| String key = iod.getProcessor(); | ||
| String value = iod.getPath(); | ||
|
|
||
| ((List<Object>) e.getInputValues().computeIfAbsent(key, k -> new ArrayList<>())).add(value); | ||
| } | ||
| // retrieves results directory | ||
| List<Object> resDirList = (List<Object>) e.getInputValues().get(RESULTS_DIRECTORY_PARAM_NAME); | ||
| if (resDirList == null) { | ||
| resDirList = new ArrayList<>(); | ||
| } | ||
| if (!resDirList.isEmpty()) { | ||
| e.setResultsLocation(resDirList); | ||
| e.getInputValues().remove(RESULTS_DIRECTORY_PARAM_NAME); | ||
| for (Map<String, Object> inputMap : e.getInputValues()) { | ||
| ((List<Object>) inputMap.computeIfAbsent(key, k -> new ArrayList<>())).add(value); | ||
|
|
||
| // retrieves results directory | ||
| List<Object> resDirList = (List<Object>) inputMap.get(RESULTS_DIRECTORY_PARAM_NAME); | ||
| if (resDirList == null) { | ||
| resDirList = new ArrayList<>(); | ||
| } | ||
| if (!resDirList.isEmpty()) { | ||
| e.setResultsLocation(resDirList); | ||
| inputMap.remove(RESULTS_DIRECTORY_PARAM_NAME); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| List<InOutData> outputs = workflowBusiness.getOutputData(s.getID(), userFolder); | ||
| for (InOutData iod : outputs) { | ||
| String key = iod.getProcessor(); | ||
|
|
@@ -315,25 +319,35 @@ public void updateExecution(Execution execution) throws VipException { | |
| } | ||
|
|
||
| public String initExecution(Execution execution) throws VipException { | ||
| Map<String, String> inputMap = new HashMap<>(); | ||
| List<Map<String, String>> inputMaps = new ArrayList<>(); | ||
| Object resultsLocation = execution.getResultsLocation(); | ||
| boolean isInputMapList = execution.getInputValues().size() > 1; | ||
| for (Map<String, Object> inputValuesMap : execution.getInputValues()) { | ||
| Map<String, String> inputMap = new HashMap<>(); | ||
| for (Entry<String, Object> restInput : inputValuesMap.entrySet()) { | ||
| if (isInputMapList && restInput.getValue() instanceof List) { | ||
| throw new VipException( | ||
| "Parameter '" + restInput.getKey() + "' contains a list, it should only have a single value when providing a list of input maps."); | ||
| } | ||
|
|
||
| for (Entry<String,Object> restInput : execution.getInputValues().entrySet()) { | ||
| inputMap.put( | ||
| restInput.getKey(), | ||
| handleRestParameter(restInput.getKey(), restInput.getValue())); | ||
| } | ||
| inputMap.put( | ||
| restInput.getKey(), | ||
| handleRestParameter(restInput.getKey(), restInput.getValue())); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if there are several inputs objets, then we must verify all inputs have only 1 element.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an exception throw |
||
|
|
||
| // We handle resultsLocation the same as others restInputs, since it can either be a String or a List<String> | ||
| Object resultsLocation = execution.getResultsLocation(); | ||
| if (resultsLocation != null) { | ||
| inputMap.put( | ||
| CoreConstants.RESULTS_DIRECTORY_PARAM_NAME, | ||
| handleRestParameter(CoreConstants.RESULTS_DIRECTORY_PARAM_NAME, resultsLocation)); | ||
| // We handle resultsLocation the same as others restInputs, since it can either be a String or a List<String> | ||
| if (resultsLocation != null) { | ||
| inputMap.put( | ||
| CoreConstants.RESULTS_DIRECTORY_PARAM_NAME, | ||
| handleRestParameter(CoreConstants.RESULTS_DIRECTORY_PARAM_NAME, resultsLocation)); | ||
| } | ||
|
|
||
| inputMaps.add(inputMap); | ||
| } | ||
|
|
||
| checkInputExecNameIsValid(execution.getName()); | ||
| return initExecution( | ||
| execution.getPipelineIdentifier(), inputMap, execution.getTimeout(), | ||
| execution.getPipelineIdentifier(), inputMaps, execution.getTimeout(), | ||
| execution.getName(), execution.getStudyIdentifier()); | ||
| } | ||
|
|
||
|
|
@@ -378,7 +392,7 @@ private void checkInputExecNameIsValid(String input) throws VipException { | |
| } | ||
|
|
||
| private String initExecution(String pipelineId, | ||
| Map<String, String> inputValues, | ||
| List<Map<String, String>> inputValues, | ||
| Integer timeoutInSeconds, | ||
| String executionName, | ||
| String studyId) throws VipException { | ||
|
|
@@ -400,40 +414,51 @@ private String initExecution(String pipelineId, | |
| if (pp.isReturnedValue()) { | ||
| continue; | ||
| } | ||
|
|
||
| List<Map<String, String>> mapsWithoutKey = inputValues.stream() | ||
| .filter(inputMap -> !inputMap.containsKey(pp.getName())) | ||
| .toList(); | ||
|
|
||
| // ok if input is present | ||
| if (inputValues.get(pp.getName()) != null) { | ||
| continue; | ||
| } | ||
| // then ok if input has a default value (and we set it) | ||
| if (pp.getDefaultValue() != null) { | ||
| inputValues.put(pp.getName(), pp.getDefaultValue().toString()); | ||
| if (mapsWithoutKey.isEmpty()) { | ||
| continue; | ||
| } | ||
| // then ok if it is optional | ||
| if (pp.isOptional()) { | ||
| continue; | ||
|
|
||
| for (Map<String, String> inputMap : mapsWithoutKey) { | ||
| // then ok if input has a default value (and we set it) | ||
| if (pp.getDefaultValue() != null) { | ||
| inputMap.put(pp.getName(), pp.getDefaultValue().toString()); | ||
| continue; | ||
| } | ||
| // then ok if it is optional | ||
| if (pp.isOptional()) { | ||
| continue; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it does not handle the case if some input object have a value and some not.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, modified to handle only missing key values
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it more logic to do the test before the loop, like:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes it makes sense |
||
|
|
||
| // error : pp is an empty input with no default value and it is not optional | ||
| logger.error("Error initialising {}, missing {} parameter", pipelineId, pp.getName()); | ||
| throw new VipException(ApiError.INPUT_FIELD_MISSING, pp.getName()); | ||
| } | ||
| // error : pp is an empty input with no default value and it is not optional | ||
| logger.error("Error initialising {}, missing {} parameter", pipelineId, pp.getName()); | ||
| throw new VipException(ApiError.INPUT_FIELD_MISSING, pp.getName()); | ||
| } | ||
|
|
||
| // fill in overriddenInputs from explicit inputs | ||
| Map<String, String> overriddenInputs = p.getOverriddenInputs(); | ||
| if (overriddenInputs != null) { | ||
| for (String key : overriddenInputs.keySet()) { | ||
| String value = overriddenInputs.get(key); | ||
| if (inputValues.containsKey(value)) { | ||
| inputValues.put(key, inputValues.get(value)); | ||
| } else { | ||
| logger.error("Error initialising {}, missing {} parameter", pipelineId, value); | ||
| throw new VipException(ApiError.INPUT_FIELD_MISSING, value); | ||
| for (Map<String, String> inputMap : inputValues) { | ||
| if (inputMap.containsKey(value)) { | ||
| inputMap.put(key, inputMap.get(value)); | ||
| } else { | ||
| logger.error("Error initialising {}, missing {} parameter", pipelineId, value); | ||
| throw new VipException(ApiError.INPUT_FIELD_MISSING, value); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| boolean inputsContainsResultsDirectoryInput = inputValues | ||
| .containsKey(CoreConstants.RESULTS_DIRECTORY_PARAM_NAME); | ||
| boolean inputsContainsResultsDirectoryInput = inputValues.stream() | ||
| .allMatch(inputMap -> inputMap.containsKey(CoreConstants.RESULTS_DIRECTORY_PARAM_NAME)); | ||
| boolean pipelineHasResultsDirectoryInput = p.getParameters().stream() | ||
| .anyMatch(param -> param.getName().equals(CoreConstants.RESULTS_DIRECTORY_PARAM_NAME)); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,13 @@ | ||
| package fr.insalyon.creatis.vip.api.model; | ||
|
|
||
| import java.util.*; | ||
|
|
||
| import jakarta.validation.constraints.NotNull; | ||
|
|
||
| import com.fasterxml.jackson.databind.annotation.JsonDeserialize; | ||
|
|
||
| import fr.insalyon.creatis.vip.api.model.serializing.InputValuesDeserializer; | ||
|
|
||
| public class Execution { | ||
|
|
||
| private String identifier; | ||
|
|
@@ -13,7 +18,8 @@ public class Execution { | |
| private int timeout; | ||
| private ExecutionStatus status; | ||
| @NotNull | ||
| private Map<String, java.lang.Object> inputValues; | ||
| @JsonDeserialize(using = InputValuesDeserializer.class) | ||
| private List<Map<String, Object>> inputValues; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to not break the API with this change, and support an object as before, and a list.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created InputValuesDeserializer which handles deserializing to List<Map<String,Object>> |
||
| private Map<String, List<java.lang.Object>> returnedFiles; | ||
|
|
||
| // optional arguments | ||
|
|
@@ -25,7 +31,7 @@ public class Execution { | |
| private Map<Integer, Map<String, Object>> jobs; // jobId -> status | ||
|
|
||
| public Execution() { | ||
| inputValues = new HashMap<>(); | ||
| inputValues = new ArrayList<>(); | ||
| returnedFiles = new HashMap<>(); | ||
| jobs = new HashMap<>(); | ||
| } | ||
|
|
@@ -94,11 +100,11 @@ public void setStatus(ExecutionStatus status) { | |
| this.status = status; | ||
| } | ||
|
|
||
| public Map<String, java.lang.Object> getInputValues() { | ||
| public List<Map<String, Object>> getInputValues() { | ||
| return inputValues; | ||
| } | ||
|
|
||
| public void setInputValues(Map<String, java.lang.Object> inputValues) { | ||
| public void setInputValues(List<Map<String, Object>> inputValues) { | ||
| this.inputValues = inputValues; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package fr.insalyon.creatis.vip.api.model.serializing; | ||
|
|
||
| import com.fasterxml.jackson.core.JsonToken; | ||
| import com.fasterxml.jackson.databind.JavaType; | ||
| import com.fasterxml.jackson.databind.deser.std.StdDeserializer; | ||
| import com.fasterxml.jackson.core.JsonParser; | ||
| import com.fasterxml.jackson.databind.DeserializationContext; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class InputValuesDeserializer extends StdDeserializer<List<Map<String, Object>>> { | ||
|
|
||
| public InputValuesDeserializer() { | ||
| super(List.class); | ||
| } | ||
|
|
||
| @Override | ||
| public List<Map<String, Object>> deserialize(JsonParser p, DeserializationContext ctx) throws IOException { | ||
| JavaType mapType = ctx.getTypeFactory().constructMapType(Map.class, String.class, Object.class); | ||
| // Array of dictionaries | ||
| if (p.currentToken() == JsonToken.START_ARRAY) { | ||
| List<Map<String, Object>> list = new ArrayList<>(); | ||
| p.nextToken(); | ||
| while (p.currentToken() != JsonToken.END_ARRAY) { | ||
| list.add(ctx.readValue(p, mapType)); | ||
| p.nextToken(); | ||
| } | ||
|
|
||
| return list; | ||
| // Unique dictionary | ||
| } else if (p.currentToken() == JsonToken.START_OBJECT) { | ||
| Map<String, Object> map = ctx.readValue(p, mapType); | ||
| return Collections.singletonList(map); | ||
| } else { | ||
| throw ctx.wrongTokenException(p, List.class, JsonToken.START_ARRAY, | ||
| "InputValues must be a dictionary or an array of dictionaries"); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| { | ||
| "name" : "Exec test 1 - modified", | ||
| "pipelineIdentifier" : "application 1/4.2", | ||
| "inputValues" : { | ||
| "param 1" : "test text", | ||
| "param 2" : "/path/test" | ||
| } | ||
| "inputValues" : [ | ||
| { | ||
| "param 1" : "test text", | ||
| "param 2" : "/path/test" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| { | ||
| "name" : "Exec test 1", | ||
| "pipelineIdentifier" : "test application/4.2", | ||
| "inputValues" : { | ||
| "testFileInput" : "/vip/Home/path/to/input.in", | ||
| "testTextInput" : "best test text value" | ||
| } | ||
| "inputValues" : [ | ||
| { | ||
| "testFileInput" : "/vip/Home/path/to/input.in", | ||
| "testTextInput" : "best test text value" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think
e.getInputValues()is empty at this point.Anyway we have to solve the workflows-db issue before (see the moteurlite PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some null checks were introduced by the latest PR