ENH: Add Modules/Beta/ container (infra-only for stacked beta-module ingests)#6085
ENH: Add Modules/Beta/ container (infra-only for stacked beta-module ingests)#6085hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
Conversation
afbf5a2 to
88522d6
Compare
| # | ||
| # In-tree home for modules that were formerly configure-time remote | ||
| # fetches (Modules/Remote/*.remote.cmake). Each module lives at | ||
| # Modules/Beta/<Name>/ with its own itk-module.cmake and is discovered |
There was a problem hiding this comment.
I would welcome suggestions for the naming of this new subdirectory. I chose "Beta," but I don't have a strong affinity for it. Perhaps "Community", "PreRelease", "Contributed", .... ?
I like the overall simplicity of this scheme. It's extensible, provides containment for items we don't yet want to put into the primary core, and provides historical context as we move some elements into the core.
There was a problem hiding this comment.
Why are they not added to their appropriate group / module according to their topic? We want to make modules simpler and more maintainable. And easier for libraries clients to use and find content.
There was a problem hiding this comment.
TLDR; Shortest path to reduced maintenance burden.
======
I was trying to keep the scope of this initial step small. The primary concern at the moment was reducing the maintainer burden by moving all code for modules primarily maintained by the core ITK developers into ITK. Thus, greatly reduces the number of repositories that need to be touched for every style, code, or CI change.
Using this beta directory adjacent to the review directory allows for the minimum amount of infrastructure change to achieve the goal of reducing maintenance burden.
Moving them from Remote or Beta to a location where clients can more easily find their content will likely involve extensive community discussions about each module: about testing code coverage, code quality, whether they have broad community appeal, ITK style conformance, and other concerns that will likely take a longer time frame to resolve per module.
This step is nearly all mechanical. I was hoping that this would allow 1/2 - 3/4 of the current remote modules to be sucked into ITK's primary repo quickly. The build behavior does not change, and it should be backward compatible with external tools that currently enable a subset of Remote modules.
There was a problem hiding this comment.
@thewtex If you think the correct path is to move directly from remote modules to direct ITK integration, would you recommend a single PR per remote module, 1 per day over the next month or two?
I'll make a trial remote module -> Direct ITK inclusion draft PR for Cuberille as a discussion point to contrast with the proposed method here.
There was a problem hiding this comment.
The primary concern at the moment was reducing the maintainer burden by moving all code for modules primarily maintained by the core ITK developers into ITK.
I agree with this goal.
Using this beta directory adjacent to the review directory allows for the minimum amount of infrastructure change to achieve the goal of reducing maintenance burden.
This duplicates code, in an unstable location, and at location that is not logically connected with existing groups and modules. This is increasing maintenance burden.
Many of the remote modules have not changed for years, have at least decent test coverage, and are not going to change. Some are easier to identify than others on which Group and Module (or a new module) they belong in. Some are trivial to locate. I do not agree that another beta group is better than putting it in the appropriate location.
@hjmjohnson I agree with you that a single PR per remote module is what is going to improve maintenance burden. It does not have to be all of them -- just the most obvious ones will make a difference on the maintenance burden and beneficial impact on the community. Then mark the remote module repository Archived on GitHub.
I'll make a trial remote module -> Direct ITK inclusion draft PR for Cuberille as a discussion point to contrast with the proposed method here.
Thank you!
|
| Filename | Overview |
|---|---|
| CMakeLists.txt | Adds add_subdirectory(Modules/Beta) in the correct position between Modules/Remote and ITKModuleEnablement; no other changes. |
| Modules/Beta/CMakeLists.txt | New file establishing the Beta module container: defines a no-op itk_beta_module_manifest() function and globs/includes *.beta.cmake provenance manifests; only minor improvement possible (CONFIGURE_DEPENDS). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[root CMakeLists.txt] --> B[add_subdirectory Modules/Remote]
A --> C[add_subdirectory Modules/Beta NEW]
A --> D[include ITKModuleEnablement]
B --> B1["file(GLOB *.remote.cmake)\nforeach → include()"]
C --> C1["define itk_beta_module_manifest() no-op"]
C --> C2["file(GLOB *.beta.cmake)\nforeach → include() manifests"]
D --> E["GLOB_RECURSE */*/*/itk-module.cmake\n(discovers Modules/Beta/Name/ automatically)"]
E --> F[Module DAG build + enablement]
Reviews (1): Last reviewed commit: "ENH: Add Modules/Beta/ container for inl..." | Re-trigger Greptile
|
This may be been done some place, but I'd like to see a categorization of remote module with considerations to integration. Potential things to consider:
|
A partial categorization list exists in #6060. You bring up some good things to consider in expanding the 6060 preliminary categorization. |
Introduces Modules/Beta/ as the in-tree home for modules that were formerly configure-time remote fetches (Modules/Remote/*.remote.cmake). Modules/Beta/CMakeLists.txt defines a no-op itk_beta_module_manifest() function so that provenance files (Modules/Beta/<Name>.beta.cmake) are valid CMake and can be include()-d without build-time side effects. Actual modules grafted under Modules/Beta/<Name>/ are discovered via the existing itk-module.cmake DAG scan in ITKModuleEnablement.cmake. No modules are ingested by this commit; this only wires the container.
88522d6 to
3f2a96a
Compare
|
@blowekamp good points — added an integration-difficulty axis as a complement to the domain axis in #6060. The full table is also in the PR body's
The 10 modules slated for ingest in #6086 are all Tier A (pure-ITK, no third-party deps, no CLI tools), which is what makes the |
|
The current "Review" module is under a "NonUnit" group. I believe the this imposes certain restrictions such as not ITK module can depend on the Review Module, and it may add all or certain dependencies. Is this a need to codify a similar restriction for the Beta group? ITK's cmake is in a bit of a different world now with the dependencies being specified directly in CMake now as interface libraries, and not just at the module level. |
Just wanted to shout out to you with a Thank You! for all your work to make that a reality! |
I think we can all agree that we want to avoid the Review / NonUnit situation, if possible. |
|
This option is also rejected. Going straight to direct integration. |
Infrastructure-only scaffold: adds
Modules/Beta/as the in-tree homefor modules formerly fetched via
Modules/Remote/*.remote.cmake. Nomodules are ingested by this PR — see the follow-on PR for the actual
ingests.
Stack: this is the base of a two-PR stack.
Modules/Beta/container only.STYLE:cleanupand per-module
COMP: Fix pre-commit hook failures for <Module>commits. Full upstream git history and blame preserved via
git merge --allow-unrelated-histories --no-ffoffilter-repo'dhistories.
Closes prior rejected approach in #6061; cross-linked to issue #6060.
What this PR adds
Modules/Beta/CMakeLists.txt— declaresitk_beta_module_manifest()(no-op CMake function) and globs
Modules/Beta/*.beta.cmakeprovenance manifests so they are valid CMake and can be include()-d
with no build-time side effects. Modules grafted under
Modules/Beta/<Name>/are auto-discovered by the existingitk-module.cmakeDAG scan inCMake/ITKModuleEnablement.cmake.add_subdirectory(Modules/Beta)into the rootCMakeLists.txtdirectly after
Modules/Remote.35 insertions across 2 files. No modules activated.
Why this shape — reviewer-facing strategy
Reviewers on #6061 rejected the "consolidate remote modules into a
category repo" approach. This restart inverts the strategy:
Modules/Beta/<Name>/, bringingtheir full upstream git history with them (so
git blamestill walksback to the original authors).
Modules/Beta/<Name>.beta.cmakethat records UPSTREAM_URL,UPSTREAM_SHA, and INGEST_DATE — the bridge lets tooling audit what
was grafted without parsing README prose.
container mechanism can land and be reviewed independently of any
per-module content.
Remote-module categorization (response to @blowekamp)
The original domain-based grouping lives in
#6060
(IO / Filtering / Mesh / Analysis / Registration). That axis sorts
modules by what they compute. @blowekamp asked for an orthogonal
integration-difficulty axis — what makes a module easy or hard to
ingest into ITK proper. The two axes are complementary; ingest
ordering should follow the difficulty axis (easiest first).
itk::types; no third-party libraries; no executables. Cleanest mechanical ingest.AnisotropicDiffusionLBR,FastBilateral,LabelErodeDilate,MorphologicalContourInterpolation,MultipleImageIterator,ParabolicMorphology,PhaseSymmetry,SmoothingRecursiveYvvGaussianFilter,SplitComponents,MeshNoise,SubdivisionQuadEdgeMeshFilterIOMeshSTL,IOMeshSWC,MeshToPolyData,BSplineGradient,Cuberille,HigherOrderAccurateGradient,GenericLabelInterpolator,RLEImageThirdParty/subtree, license review, and afind_packagestory.AnalyzeObjectLabelMap,IOFDF,IOMeshMZ3,IOOpenSlide,IOScanco,IOTransformDCMTK,MGHIO,IOOMEZarrNGFF,SCIFIO,TotalVariation,VkFFTBackendPerformanceBenchmarking,IsotropicWavelets(CLI),Strain(CLI),MinimalPathExtraction(CLI),SkullStrip(CLI)RTK,CudaCommon(RTKConsortium),SimpleITKFilters(SimpleITK),ITKElastix-tracked modulesSkullStrip,LesionSizingToolkit,Montage,TubeTK,Ultrasound,HASI,BoneMorphometry,BoneEnhancementCleaver,AdaptiveDenoising,SCIFIO,Shape,TractographyTRX,CudaCommonSphinxExamples,WebAssemblyInterface(toolchain)BioCell,IOOMEZarrNGFF(defer),TotalVariation(defer)This PR's
Modules/Beta/scope = Tier A only. The 10 modulesingested in the follow-on PR #6086 are all Tier A (pure-ITK, no third-
party deps, no executables, no external release cycle, no large
application surface). Tiers B–I will be handled later, each with its
own per-tier scoping discussion. The provenance manifest at
Modules/Beta/<Name>.beta.cmakerecords UPSTREAM_URL/SHA so a futuremove from Beta back out (e.g., promoting to a stable module group, or
demoting to remote because of newly added third-party deps) is fully
auditable.