Parameter serializer support #576
Conversation
| # Install JsonCpp | ||
| # ------------------------------------------------------------------------- | ||
| if(Slicer_BUILD_PARAMETERSERIALIZER_SUPPORT | ||
| AND NOT "${JsonCpp_DIR}" STREQUAL "" AND EXISTS "${JsonCpp_DIR}/CMakeCache.txt") |
There was a problem hiding this comment.
Note for later, I created open-source-parsers/jsoncpp#526 to associate components with JsonCpp install rules
|
Very nice 👍 Beside of a small nitpick in the test, looks good note that since the PR updates SuperBuild files, you should not expect the CI to be green. I will also try to build locally and report back |
SuperBuild/External_JsonCpp.cmake
Outdated
|
|
||
| ExternalProject_Add(${proj} | ||
| ${${proj}_EP_ARGS} | ||
| GIT_REPOSITORY "${git_protocol}://github.com/KitwareMedical/jsoncpp.git" |
There was a problem hiding this comment.
Are the differences of this from upstream summarized somewhere?
I really don't like the divergence from the main repository.
open-source-parsers/jsoncpp#467 mentioned in KitwareMedical/jsoncpp#2 has been merged long time ago.
There was a problem hiding this comment.
Good point.
I started to work on contributing some of the changes upstream ... see open-source-parsers/jsoncpp#467 . A fresh look on it would be nice.
@vovythevov Make sure to document different in the commit message. We should also properly name the branch in KitwareMedical/jsoncpp
There was a problem hiding this comment.
@jcfr I am not clear about your comment. open-source-parsers/jsoncpp#467 is integrated.
Are you saying that you have your comment open-source-parsers/jsoncpp#467 (comment) about bumping up minimum version of cmake addressed too? Is that the unresolved issue?
There was a problem hiding this comment.
@jcfr I am not clear about your comment. open-source-parsers/jsoncpp#467 is integrated.
Great. This was on oversight. In that case, we should have look at the other commits to also integrate them if relevant.
My other comment was some suggestion to alleviate the concern raised.
We are all set then.
The two other commits are quit minor.
@vovythevov Could you look into using the latest release of jsoncpp ?
There was a problem hiding this comment.
Excellent!
Note this https://github.com/open-source-parsers/jsoncpp/tree/0.y.z#a-note-on-backward-compatibility. Should we use 0.y.z branch?
In dcmqi, we use 0.y.z branch (amalgamated version of 0.y.z, to be precise), since I thought C++11 is not supported in Slicer.
There was a problem hiding this comment.
Version 0.10.6 should indeed be used
since I thought C++11 is not supported in Slicer.
That is correct. It is currently not supported.
johan-andruejol
force-pushed
the
ParameterSerializerSupport
branch
from
40341a9 to
3fccb7e
Compare
|
Looks good 👍 I am building locally and will report shortly. |
|
Let me know if you haven't tested on Mac and would like me to do that. |
|
@fedorov If you have the time, doing a build enabling option @vovythevov Look like the test if failing on Linux: |
|
@vovythevov When comparing the output, it is normal to have different memory address ... you should make the comparison more robust. |
|
Adding The error is this one: |
|
@jcfr: This actually caught an error, I did not see that message on Did you build in Debug mode ? I am thinking that this is a Debug output On Wednesday, August 31, 2016, Jean-Christophe Fillion-Robin <
Sent from Gmail Mobile |
It is
The error is always reported No particular rush, let me know what you find out later tomorrow. |
|
No build errors on mac 10.10.5 |
|
While trying to compile an external code against SlicerExecutionModel built from this branch, I ran into the following error: I believe this may be related to the changes introduced, since this code used to compile with the older Slicer. The code I tried to compile is here: https://github.com/qiicr/Iowa2DICOM |
|
@fedorov: Thanks, this is really helpful ! I'll look into this. |
|
@jcfr, @fedorov: Thanks for you help guys ! Your feedback helped to dig some bugs so that was really good. I addressed them in the SlicerExecutionModel PR# 77 (Slicer/SlicerExecutionModel#77). As soon as this is merged, I'll update this PR with the new hash and ping you. |
CMakeLists.txt
Outdated
| "Slicer_BUILD_ITKPython" OFF) | ||
| mark_as_superbuild(Slicer_INSTALL_ITKPython) | ||
|
|
||
| option(Slicer_BUILD_PARAMETERSERIALIZER_SUPPORT "Build Slicer with parameter serializer support" OFF) |
There was a problem hiding this comment.
Why is it OFF by default?
This means we won't have jsoncpp in the nightly, and we will need it in the default configuration for some of the things done in the Segmentations module (at least).
Cc: @cpinter
There was a problem hiding this comment.
After a first round of nightly with the option OFF, we will turn it ON
There was a problem hiding this comment.
Note that jsoncpp is shipped with VTK as well. In the Slicer core we should rather depend on VTK only (Slicer may be built using CLI support disabled), unless there is a strong reason against it.
Is the jsoncpp version in VTK will be synchronized with jsconcpp in SlicerExecutionModel? Or maybe we can build Slicer's VTK with an external jsoncpp (provided by Slicer superbuild).
There was a problem hiding this comment.
Certainly the SlicerExecutionModel should not depend on VTK. We are talking about how Slicer modules will access jsoncpp and how to make sure the same jsoncpp version is used by SlicerExecutionModel, VTK, and Slicer modules. One solution could be that Slicer superbuild builds jsoncpp and passes it to both SlicerExecutionModel and VTK.
There was a problem hiding this comment.
Andras, yes I agree. If there is something to be improved it is factoring out jsoncpp from VTK.
There was a problem hiding this comment.
VTK can already accept an external jsoncpp (VTK_USE_SYSTEM_JSONCPP option).
There was a problem hiding this comment.
Nice. Then they should definitely use the same jsoncpp build.
johan-andruejol
force-pushed
the
ParameterSerializerSupport
branch
from
3fccb7e to
6ca130e
Compare
|
@fedorov, @jcfr: Pushed the updated branch.
|
|
Thanks 👍 Locally building now. |
|
This is a release build / Ubuntu 15.10 |
|
|
And the corresponding Json: $ cat /home/jcfr/Projects/Slicer-Release/Slicer-build/Testing/Temporary/ExecutionModelTourSerialized.json
{
"Parameters" :
{
"Boolean Parameters" :
{
"boolean1" : true,
"boolean2" : false,
"boolean3" : false
},
"Enumeration Parameters" :
{
"stringChoice" : "Bill"
},
"File, Directory and Image Parameters" :
{
"directory1" : "",
"file1" : "",
"files" : [ "1.does", "2.not", "3.matter" ],
"image1" : "",
"image2" : "",
"outputFile1" : "",
"seed" :
[
[ 1, 0, -1 ]
],
"seedsFile" : "",
"seedsOutFile" : "/home/jcfr/Projects/Slicer-Release/Slicer-build/Testing/Temporary/SeedsSerialized.acsv",
"transform1" : "/home/jcfr/Projects/Slicer/Testing/Data/Input/ExecutionModelTourTest.mrml#vtkMRMLLinearTransformNode1",
"transform2" : "/home/jcfr/Projects/Slicer/Testing/Data/Input/ExecutionModelTourTest.mrml#vtkMRMLLinearTransformNode2"
},
"Generic Tables" :
{
"inputDT" : "",
"outputDT" : ""
},
"Index Parameters" :
{
"arg0" : "/home/jcfr/Projects/Slicer/Testing/Data/Input/MRHeadResampled.nhdr",
"arg1" : "/home/jcfr/Projects/Slicer/Testing/Data/Input/CTHeadAxial.nhdr"
},
"Measurements" :
{
"inputFA" : "",
"outputFA" : ""
},
"Regions of interest" :
{
"regions" : []
},
"Scalar Parameters" :
{
"doubleVariable" : 30,
"integerVariable" : 30
},
"Simple return types" :
{
"abooleanreturn" : false,
"adoublereturn" : 14,
"afloatreturn" : 7,
"anintegerreturn" : 5,
"anintegervectorreturn" : [],
"astringchoicereturn" : "Bill",
"astringreturn" : "Hello"
},
"Vector Parameters" :
{
"floatVector" : [ 1.2999999523162842, 2, -14 ],
"stringVector" : [ "foo", "bar", "foobar" ]
}
}
} |
|
Good news. The issue was that the code wasn't regenerated ... forcing a clean build ensure the test pass |
|
side note, there are few warnings: |
|
It turns out that |
|
Investigating further with @vovythevov , it turns out that within SlicerExecutionModel, modifiying the source |
|
@jcfr: It's ugly, but it works: |
$ git shortlog --no-merges 26a62a0b..9a74e80a
Jean-Christophe Fillion-Robin (7):
STYLE: CMakeLists: Consistently requires version 2.8.6
STYLE: CMakeLists: Simplify code removing unneeded setting of CMP0016 to NEW
STYLE: CMakeLists: Simplify code removing unneeded setting of CMP0017
STYLE: CMakeLists: Simplify buildsystem using FindThreads module
STYLE: CMakeLists: Remove obsolete option ModuleDescriptionParser_USE_PYTHON
STYLE: CMakeLists: Finding of threading library is not needed anymore
JsonSerializationUtilities: Remove unused method
Johan Andruejol (10):
ENH: Improve JsonCpp support for SlicerExecutionModel
ENH: Add JsonCpp path to the GenerateCLP launcher
ENH: Do not pass the JsonCpp_DIR to the testing directly
ENH: Allow CLI to deserialize without the required arguments
ENH: Improve support for empty arrays
ENH: ParameterSerializer: Use the longflag in priority
ENH: ParameterSerializer: Fix multiple parameters handling for deserialization
STYLE: GenerateCLP: Factorize out the CMakeLists of the testing CLIs
ENH: ParameterSerializer: Improve escaping of the JSONModuleDescription
ENH: GenerateCLP: Force GenerateCLPLauncher rebuild on GenerateCLP change
This allows Slicer to build JsonCpp. This is part of the integration of the parameter serializer effort. By default, the JsonCpp support is disabled.
With the paramater serializer support, one can now serialize and deserialize CLIs with the command line. The python test CLISerializationTest is added to make sure it works as expected.
johan-andruejol
force-pushed
the
ParameterSerializerSupport
branch
from
6ca130e to
2dc535c
Compare
|
@jcfr, @fedorov: Latest update with all the recent changes in SlicerExecutionModel:
|
|
Nice 👍 Could you also confirm that https://github.com/qiicr/Iowa2DICOM build ? |
|
Re-building after:
fails with It turns out that on single config system the correct path is: Note that Release is not present |
SuperBuild/External_JsonCpp.cmake
Outdated
| else() | ||
| set(lib_ext "so") | ||
| endif() | ||
| set(${proj}_LIBRARY ${${proj}_DIR}/src/lib_json/$<CONFIG>/jsoncpp.${lib_ext}) |
There was a problem hiding this comment.
Consider this instead:
set(${proj}_LIBRARY ${${proj}_DIR}/src/lib_json/${CMAKE_CFG_INTDIR}/jsoncpp.${lib_ext})
There was a problem hiding this comment.
Prefix of the lib is also incorrect on unix, this patch seems to work:
diff --git a/SuperBuild/External_JsonCpp.cmake b/SuperBuild/External_JsonCpp.cmake
index 724f01d..f3a53ea 100644
--- a/SuperBuild/External_JsonCpp.cmake
+++ b/SuperBuild/External_JsonCpp.cmake
@@ -60,14 +60,17 @@ if(NOT DEFINED ${proj}_DIR AND NOT ${CMAKE_PROJECT_NAME}_USE_SYSTEM_${proj})
set(${proj}_DIR ${CMAKE_BINARY_DIR}/${proj}-build)
set(${proj}_SOURCE_DIR ${CMAKE_BINARY_DIR}/${proj})
set(${proj}_INCLUDE_DIR ${${proj}_SOURCE_DIR}/include)
- if (WIN32)
+ if(WIN32)
+ set(lib_prefix "")
set(lib_ext "lib")
elseif(APPLE)
+ set(lib_prefix "lib")
set(lib_ext "dylib")
else()
+ set(lib_prefix "lib")
set(lib_ext "so")
endif()
- set(${proj}_LIBRARY ${${proj}_DIR}/src/lib_json/$<CONFIG>/jsoncpp.${lib_ext})
+ set(${proj}_LIBRARY ${${proj}_DIR}/src/lib_json/${CMAKE_CFG_INTDIR}/${lib_prefix}jsoncpp.${lib_ext})
#-----------------------------------------------------------------------------
# Launcher setting specific to build treeFor consistency the JsonCpp variables passed to the projects were changed
to JsonCpp_INCLUDE_DIR and JsonCpp_LIBRARY.
$ git shortlog 9a74e80ac..8692b09b6 --no-merges
Johan Andruejol (1):
ENH: Uniformize the use of JsonCpp with VTK
johan-andruejol
force-pushed
the
ParameterSerializerSupport
branch
from
9bcc554 to
a8aff8b
Compare
|
@jcfr: Thanks for the (very quick) patch ! I can confirm that the error that @fedorov reported Slicer/Slicer#576 (comment) is no longer present. Howver, the build fails on my machine for unrelated linking issues, because I don't have the xml2 and z libraries in my path on windows. See: https://github.com/QIICR/Iowa2DICOM/blob/master/ConvertSegmentations/CMakeLists.txt#L7 The error: |
|
After confirming it compiles and packages on MacOSX, LGTM |
Have you tried commenting out those libs? It may or may not help - I have never tried to build that code on windows, and it won't be maintained, since the functionality migrates to dcmqi. Thanks for resolving these remaining issues! |
@vovythevov I confirm that MacOSX packaging works as expected. Since configuring VTK with jsoncpp is not used/tested, it is only used in Tomorrow, if dashboard looks reasonable, consider doing an other commit that switch it to Thanks |
|
Very nice 👍 Note: I just started to rebuild the SlicerDocker images |
|
Great, thank you! I'll try it right away! |
|
@cpinter: Note that it's not on by default for now, make sure to turn Slicer_BUILD_PARAMETERSERIALIZER_SUPPORT on. |
The plan is to enable Slicer_BUILD_PARAMETERSERIALIZER_SUPPORT by default tomorrow |
The parameter serializer support allows CLI to serialize/deserialize their input from and to JSON file. For example, with the GaussianBlurImageFilter one could do the following: # Serialize, i.e. save the parameters to JSON: ./Slicer --lauunch GaussianBlurImageFilter MRHead.nrrd BlurredHead.nrrd --serialize GaussianBlurParameters.json # Deserialize, i.e. load the parameters from JSON : ./Slicer --launch GaussianBlurImageFilter --deserialize GaussianBlurParameters.json This follows the changes made in r25335, r25336 and r25337.
|
@jcfr: I pushed a commit to turn on the parameter serializer by default: Slicer/Slicer@a095ced. |
|
LGTM 👍 |
|
Well done! |
6 participants
@fedorov, @jcfr, @thewtex: This is the pull request to add ParameterSerializer and the JsonCpp support.
This PR also adds a test to make sure we can properly serialize/deserialize a CLI with Slicer.
Now, someone can serialize their CLI by doing:
and re-run that CLI with the same arguments using only the JSON file: