Skip to content
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

ENH: Migrate data to IPFS #398

Merged
merged 2 commits into from
Nov 14, 2022
Merged

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Nov 7, 2022

No description provided.

@github-actions github-actions bot added area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module area:Documentation Issues affecting the Documentation module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Nonunit Issues affecting the Nonunit module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Data Changes to example data (usually displayed images) type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels Nov 7, 2022
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Is kicking out sha512s a WIP simplification, or a longer-term plan?

@thewtex
Copy link
Member Author

thewtex commented Nov 7, 2022

Is kicking out sha512s a WIP simplification, or a longer-term plan?

Both

@thewtex thewtex force-pushed the data-cid branch 3 times, most recently from eded441 to 284281a Compare November 10, 2022 03:39
@thewtex thewtex changed the title WIP: ENH: Migrate data to IPFS ENH: Migrate data to IPFS Nov 11, 2022
@thewtex thewtex marked this pull request as ready for review November 11, 2022 14:41
@thewtex
Copy link
Member Author

thewtex commented Nov 11, 2022

This improves build reliability, addressing #395, but also simplifies the contributor experience and improves maintainability and sustainability.

Documentation updates were factored into a separate commit to facilitate review.


#]=======================================================================]

function(ExternalData_add_test target)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly necessary, but it would be helpful to have a comment on each function summarizing intended behavior and detailing input formats and example usage. I.e. # Usage: ExternalData_add_test(ReadAnImageData), etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, although we do not want to diverge from upstream ExternalData.cmake.

Copy link
Contributor

Choose a reason for hiding this comment

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

we do not want to diverge from upstream ExternalData.cmake

It doesn't seem like this constraint will be easy to enforce on a forked file here. Is there another way we could reference ExternalData.cmake from an existing ITK source dir or fetch it from an ITK hash with FetchContent at build time?

Copy link
Member

Choose a reason for hiding this comment

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

We could require a newer version of CMake which does have IPFS support in ExternalData. Matt, do you know which is the oldest version good enough for this?

Copy link

@jcfr jcfr Nov 11, 2022

Choose a reason for hiding this comment

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

To clarify, the IPFS support has not yet been contributed to upstream CMake 1

Waiting we integrate it upstream, an intermediate approach would be to maintain the modified ExternalData module into a project called for example cmake-IPFS-ExternalData (or just cmake-ExternalData) maintained in the @InsightSoftwareConsortium GitHub organization.

Structure would be similar to what we are doing for scikit-build/cmake-FindVcvars and scikit-build/cmake-FindFortran.

The proposal structure provide clear guideline for:

  • integrating the custom module in projects
  • versioning the module for easy integration

Footnotes

  1. https://github.com/Kitware/CMake/blob/master/Modules/ExternalData.cmake

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good idea. I will start a new dedicated repository for the module when integrating into the ITK repository.

Note that it intentionally has not been submitted to upstream CMake because of the current lack of availability of a C++ CID verification library.

CMake/ITKSphinxExamplesExternalData.cmake Show resolved Hide resolved
CMake/ITKSphinxExamplesExternalData.cmake Outdated Show resolved Hide resolved
This improves reliability, addressing InsightSoftwareConsortium#395, but also the contributor
experience, maintainability, and sustainability.
@github-actions github-actions bot added the type:Enhancement Improvement of existing methods or implementation label Nov 11, 2022
Copy link
Contributor

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

Thank you for adding documentation @thewtex .

Documentation/Contribute/UploadBinaryData.rst Outdated Show resolved Hide resolved
Documentation/Contribute/UploadBinaryData.rst Outdated Show resolved Hide resolved
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Thanks for all this hard work Matt 💯.

I only have one concern: why is the Documentation/Contribute/ContributeWithGit.rst file being removed?

It deals with contributing to the repository; and is not related to the data part (if any part does refer to the data, then cross-referencing Documentation/Contribute/UploadBinaryData.rst would suffice).

Last, I think we have tried keeping the line length under 79 characters for RST files. Maybe in the long run having a linter for this would make our life easier.

@thewtex
Copy link
Member Author

thewtex commented Nov 11, 2022

I only have one concern: why is the Documentation/Contribute/ContributeWithGit.rst file being removed removed?

@jhlegarreta great catch! That should not have been removed.

Last, I think we have tried keeping the line length under 79 characters for RST files. Maybe in the long run having a linter for this would make our life easier.

Yes, I went through and wrapped.

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Building the documentation is causing some probably justified warnings.

Documentation/Contribute/UploadBinaryData.rst Show resolved Hide resolved
Provide motivation and simplified contributor experience.
@thewtex thewtex merged commit 9a55e54 into InsightSoftwareConsortium:master Nov 14, 2022
@thewtex thewtex deleted the data-cid branch November 14, 2022 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Bridge Issues affecting the Bridge module area:Core Issues affecting the Core module area:Documentation Issues affecting the Documentation module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Nonunit Issues affecting the Nonunit module area:Numerics Issues affecting the Numerics module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module type:Data Changes to example data (usually displayed images) type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants