SlicerRT does not build due to missing ITK remote module

slicerrt
(Andras Lasso) #7

Nightly Slicer will be unstable for a while due to many major changes that we are planning to do (Python3…). Can your users stay on the latest stable version?

(Csaba Pinter) #8

That’s what I told them. There are not a lot of changes in SlicerRT so they don’t lose much. But we might lose users who try the nightly and they don’t find what they want.

(Jean Christophe Fillion Robin) split this topic #9

3 posts were split to a new topic: Improve description of nightly build on download.slicer.org

(Csaba Pinter) #11

After taking a better look, these errors, although there are many, do not seem hard to fix. At least for us what took the long time was to investigate what broke exactly and to find the time to start the fixes (deadlines etc). Doing the fixes should be now easy, and can be expected in a day or two.

I don’t think it is needed to revert the change unless there are multiple other extensions that rely heavily on ITK as much as SlicerRT (due to Plastimatch mainly).

1 Like
(Jean Christophe Fillion Robin) #12

Based on your finding and implemented fixes, could you add entry to https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/MigrationGuide#Transition_from_ITK4_to_ITK5

(Jean Christophe Fillion Robin) split this topic #14

A post was split to a new topic: Streamlining maintenance of download.slicer.org

(Csaba Pinter) #15

Some of the things are specific to Plastimatch (e.g. I don’t think anyone else uses BioCell), and I find that ITK migration guide is pretty good. Since there is a link already to the full migration guide I think it is enough.

(Andrey Fedorov) #16

I just saw this post about release of ITK 4.13.2, which made me quite confused about the discussion in this thread. Quoting that post,

The next feature pre-release for ITK 5, ITK 5 Release Candidate 2, is anticipated in a few weeks

If ITK 5 is not yet released, we only have release candidate 1, why is Slicer switching to ITK 5 now?

(Steve Pieper) #17

Part of the motivation was that @phcerdan was available to do the troubleshooting for the needed C++ changes. Also this has been on the roadmap for a long time to upgrade all the dependencies and we planned that the nightly would be undergoing a lot of slicer5 changes (python3 is coming soon too) and that was accepted since we have a 4.10.1 already and plans for a 4.10.2 soon for people who aren’t ready to change. I think it’s fine that we are a bit ahead of the game on ITK. I doubt it’ll change much between RC and release.

(Andrey Fedorov) #18

I think C++ changes is different from upgrading to a dependency that is in a pre-release stage. I did not find ITK 5 upgrade in the roadmap, maybe I missed it. Maybe it all makes sense to the developers closely involved, but for someone a bit on the outside, switch to a prerelease of a key dependency looks strange.

(Steve Pieper) #19

Personally I don’t think it’s strange at all that the pre-release version of Slicer would be using the almost released version of ITK. You are right that the roadmap does not specifically callout ITK5, but it makes it clear that disruptive changes are coming…

(Pablo Hernandez-Cerdan) #20

Hi @fedorov, I updated the wiki with the steps taken on the transition. ITKv4->ITKv5 and the ITKv5 Migration Guide

Upgrading external modules should be a matter of upgrading to ITKv5 new threads,
if the ThreadedGenerateData makes no use of threadId replacing:

void ThreadedGenerateData( const OutputRegionType& threadRegion, ThreadIdType threadId )

void DynamicThreadedGenerateData( const OutputRegionType& threadRegion )

will do.

In other cases, where threadId is involved, the ITK migration guide has examples, but to just use the ITKv4 threading system add to the ITK class constructor:

this->DynamicMultiThreadingOff();

And replace

  #include <itkMultiThreader.h>

  itk::MultiThreader::Pointer ProcessingThreader;

with:

  #include <itkPlatformMultiThreader.h>

  itk::PlatformMultiThreader::Pointer ProcessingThreader;

that will probably solve 99% of the problems upgrading to ITKv5.

I am sorry the upgrade forced external plugins to catch up…

1 Like
(Csaba Pinter) #21

I went through the extensions on the latest nightly dashboard that did not build. These are the extensions that fail to build due to ITK and the failure seems to be related to version 5 (and not some older issue like itkFactoryRegistration):

  • ABC
  • DSCMRIAnalysis
  • DTI-Reg
  • DTIAtlasBuilder
  • DTIProcess
  • IASEM
  • PET-IndiC
  • PETTumorSegmentation
  • PkModeling
  • ResampleDTIlogEuclidean
  • SkullStripper
  • SlicerElastix
  • SlicerVMTK
  • SPHARM
  • VirtualFractureReconstruction (also failed before for different reasons)

All of these issues are related to these few things:

  • Changed constants such as ITK_THREAD_RETURN_TYPE
  • The abovementioned multi-threading changes
  • Changed vnl functions (e.g. vnl_math_isinf or vnl_math_abs)
  • Deprecated vcl functions (e.g. vcl_cstdio.h or vcl_vector)
  • Changed image initialization ( conversion from ‘int’ to ‘const itk::SmartPointer<itk::Image<float, 3> >’ is ambiguous) - I haven’t met this one when fixing SlicerRT but I think this should be new as well

I volunteer fixing SlicerElastix as I’m planning to work on it regarding the node references I’m adding to all registration modules.

4 Likes
(Steve Pieper) #22

Thanks @cpinter! You probably know this, but @lassoan checked the elastix repo and they were actively porting to itk5 so maybe it’s just a matter of changing the superbuild hash.

1 Like
(Andrey Fedorov) #23

@chribaue @abeers FYI

(Csaba Pinter) #24

Just an update that I issued a PR to Elastix to fix the remaining build errors. Once this is integrated I’ll update the hash.

(Andras Lasso) #25

This is great, thank you! Until they integrate the fix, you can fork the Elastix repository and use that fork for nightly builds. It would allow us to start testing Elastix build on all operating systems.

(Steve Pieper) #26

Looks like there’s a small conflict between @cpinter’s patch and the one @N-Dekker did. If it doesn’t get resolved in a few days then yes, doing temporary Slicer-specific elastix fork would be a good idea.

(Csaba Pinter) #27

Just returned from travelling. I’ll take care of this tomorrow. Thanks for the patience

(Csaba Pinter) #28

This pull request fixes the build issues (only tested on Windows) and adds node references from the transform to the inputs: