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

CRAN Comments #1 #1

Open
stnava opened this issue Mar 20, 2015 · 11 comments
Open

CRAN Comments #1 #1

stnava opened this issue Mar 20, 2015 · 11 comments

Comments

@stnava
Copy link
Member

stnava commented Mar 20, 2015

[This was generated from CRAN.R-project.org/submit.html]

The following package was uploaded to CRAN:

Package Information:
Package: ITKR
Version: 0.0
Title: ITK in R
Author(s): Brian B. Avants, Benjamin M. Kandel, Jeff T. Duda
Maintainer: Brian B. Avants [email protected]
Suggests: knitr
Description: ITKR interfaces a state of the art image processing library
with R statistical methods. ITKR uses cmake to access these
frameworks from within R and, ultimately, support
reproducible statistical analyses of medical images via
interfacing with other packages.
License: GPL (>=2)

The maintainer confirms that he or she
has read and agrees to the CRAN policies.

Submitter's comment: ## Test environments

  • local OS X install, R 3.1.3
    *
    ubuntu 12.04 (on travis-ci), R 3.1.3

Checking with current R-devel is required!

R CMD check

results
There were no ERRORs or WARNINGs.

There were
2 NOTES:

  • New maintainer

  • checking installed
    package size ... NOTE
    installed size is 86.9Mb

    sub-directories of 1Mb or more:
    libs
    86.8Mb

This is a templated C++ header and medical
imaging I/O library with substantial functionality.
The package is along the lines of
BH
and has a similar memory footprint.

Downstream

dependencies
There are no downstream dependencies
except for a github package
ANTsR which we
would like to submit to CRAN after ITKR.

Response from CRAN

Hmm, BH is a service to others, not an example to you.

You really must comply with the policies you claim to have read. We see

  • checking CRAN incoming feasibility ...

Availability using Additional_repositories specification:
? ?
http://github.com/InsightSoftwareConsortium/ITK.git

Additional repositories with no packages:
http://github.com/InsightSoftwareConsortium/ITK.git

[It is not an R package repository: do not abuse the field. And there is a blank repository due to the line break in the field.]

Package has a VignetteBuilder field but no prebuilt vignette index.

The Description field should not start with the package name,
'This package' or similar.

cmake is not portable. It seems you did not submit this to winbuilder as the policies suggest. It failed to install on Solaris, which does not have cmake: I am not sure all OS X boxes do, but mine did.

In any case, downloading sources each time this is compiled would be an intolerable load on the CRAN check farm.

@jefferis
Copy link

Just a quick point about the Author(s): In this case you need to ensure that the ITK authors / license are correctly specified.

@stnava
Copy link
Member Author

stnava commented Mar 21, 2015

primary issues:

  • need to distribute ITK inside of ITKR
    • this is basically already done .... very annoying to automate but not too big a deal
  • seems unlikely to be able to use cmake ... though would be nice if we could

secondary:

  • might need to deal with windows - but shouldnt have to if above issues resolved

somewhat related: RcppCore/Rcpp#266 (comment)

@stnava
Copy link
Member Author

stnava commented Mar 21, 2015

this is perhaps ridiculous but would make ITKR and ANTsR much easier ....

https://github.com/stnava/cmaker

@dipterix
Copy link

Maybe bioconductor? There is a package rhdf5, its depending package Rhdf5lib compiles evertime. CRAN packages can also import BioC packages nowadays. (Maybe it's time to re-consider : D

@ncullen93
Copy link
Member

If cmake is not an issue anymore - should we revisit this? I think getting ITKR on CRAN could be a good first test to see if ANTsR will eventually work. I thought the size would still be an issue though?

@dipterix so both Rhdf5 and Rhdf5lib are on Bioconductor, right? Are they more lenient on size?

@dipterix
Copy link

dipterix commented Feb 16, 2024

Updated 02/16, 2024:

I'm building the package using the following layout. R is not complaining the package size anymore. However, I have to write wrappers for functions that I want to export (using [[Rcpp::export(cpp)]]. I have to clean the ITK folders (remove unused files, rename the folders so the path lengths will be less than 100 characters...), but I feel it's hopeful to get things on CRAN.

image

Makevars:

image

Original reply:

The CRAN's attitude towards package size is pretty vague. From what I know, CRAN is OK with packages > 5MB. Some packages like Rvcg and ravetools have source package size wayyy beyond the limit and yet they are still on CRAN.

I think that limitation was referring to the compiled package size. Package ravetools is under 5MB after compilation, and R-CMD-check doesn't trigger any warning.

Besides, I don't think CRAN is too strict on the package size, as long as it's not too over (like > 30MB, me guess). For example BH package is 8-12MB (depending on platforms) after compilation. I have a package in which the external engine is 6MB, and CRAN is fine with it (you just have to reason through it, and make your efforts removing unnecessary files).

My worry about ITKR is ITK itself is too large. There are too many modules inside that might not be even used by ANTsR, which makes it hard to justify. Besides the whole ITK will be included as-is in compiled package, hence making it impossible to reduce the size.

My suggestion is to trim the ITK and only leave functions that ANTsR needs, and avoid cmake: https://itk.org/Wiki/ITK/Using_ITK_Without_CMake, and write wrapping functions only for stuff that is to be used by ANTsR.

Last but not least, if CRAN is a long shot, you can use r-universe, which provides CRAN-like compiling features for all major platforms, and does not need size restrictions (you have to make sure R-CMD-check reports no error though)

@ncullen93
Copy link
Member

Yeah this looks great. When you talk about wrappers for functions you want to export, you mean you're wrapping ITK functions in R within the same package or writing a separate package and using LinkingTo: ITKR ?

In the end, what I really care about is getting ANTsR on CRAN. The way I'm working on it was to move all the C++ / compiled stuff to ITKR + another package ANTsRCore so that ANTsR is only R code. That makes ANTsR super small and CRAN-ready, but how would you recommend we incorporate those heavy packages as dependencies? Hope that question makes sense.. happy to expand.

@dipterix
Copy link

I think instead of having 3 packages, 2 packages might be a good choice. You can merge ANTsRCore with ITKR, and put C++ ITK within src/itkxxx folders. The compiled code will not contain the whole ITK, instead, only functions used by ANTs will be included in the compiled package.

One worry is CRAN only allow dynamically linked libraries to ensure portability. I'm not familiar with ITK. Are there any modules statically built in ITK?

@ncullen93
Copy link
Member

ncullen93 commented Feb 16, 2024

That's definitely a possibility, but I think it's easier to keep ITK a bit separate for now. I'm not an ITK expert either.

Flipping this question though, if we wanted to get ANTsR on CRAN while depending on ITKR/ANTsRCore that exists only on Github, do you think that would be allowed? What's the strategy? That would basically just be like a CRAN package depending on a github remote, which seems fine. The build size for ANTsR now is <1mb now that I moved everything to ANTsRCore. Only issue is the build time would so long.

Maybe we go ITKR/ANTsRCore to bioconductor if they're more lenient and then ANTsR to CRAN..?

@dipterix
Copy link

dipterix commented Feb 16, 2024

A CRAN package can only depend/import another CRAN/Bioconductor package. Even if you put ANTsRCore on bioconductor, CRAN people will still ask you about CRAN violations when submitting ANTsR to CRAN. (Yes, maintainers having packages on bioconductor may receive emails from CRAN asking for changes, even though they never submit to CRAN. If you make ANTsRCore to bioconductor, maybe you can consider making ANTsR on bioconductor too.

(Just my observations.... : )

@dipterix
Copy link

dipterix commented Feb 16, 2024

When you talk about wrappers for functions you want to export, you mean you're wrapping ITK functions in R within the same package or writing a separate package and using LinkingTo: ITKR ?

Just realized I misunderstood this one. I mean you can wrap those function in C++, and use Rcpp::interfaces(cpp) to generate interface for wrappers. Notice since you only export cpp, there is no R interface.

In src/xxx.cpp:

// [[Rcpp::interfaces(cpp)]]
void doSomething( itk::xxx ){
  blahblah;
}

Unlike the normal Rcpp::export (which is default to exposing the functions to R), Rcpp::interfaces(cpp) only expose the function into link-able headers. Rcpp will automatically create a cpp interface in inst/include (you don't need to manage it), and you can link to ITKR in other packages, and doSomething will be available to you in ANTsRCore, via #include <ITKR.h>.

In this way you don't need to include the whole ITK in your package release.

Reference: https://search.r-project.org/CRAN/refmans/Rcpp/html/interfacesAttribute.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants