gesistsa / rio

🐟 A Swiss-Army Knife for Data I/O
http://gesistsa.github.io/rio/
604 stars 76 forks source link

Support at least one binary open standard out of the box #315

Open chainsawriot opened 1 year ago

chainsawriot commented 1 year ago

Except those plain text formats, all binary formats supported by this package out of the box are proprietary formats (Excel, SAS, Stata, SPSS), provided by openxlsx, haven, and readxl. These formats are popular and I support that they should remain the default. However, a proposal is to support at least one open binary format, which is 3 vs 1. I believe it's fairer. It also allows one to convert proprietary formats to a fast but open binary format out of the box.

From our list, there are Apache Parquet, feather, fst, and OASIS ODS. I think Parquet is the ideal candidate for this because it is fast and popular. One drawback is that Desktop application for opening Parquet file is not ubiquitous. ODS on the other hand is much slower but has an edge that Excel, LibreOffice, and Google Sheets all support it.

Disclosures of Possible Conflicts of Interest: I am also the maintainer of readODS

chainsawriot commented 1 year ago

340

schochastics commented 1 year ago

To understand this correctly: this is about moving arrow or readODS from Suggests to Import? Maybe the dependencies should be taken into consideration?

rang::resolve("readODS")
#> resolved: 1 package(s). Unresolved package(s): 0 
#> $`cran::readODS`
#> The latest version of `readODS` [cran] at 2023-08-14 was 2.0.0, which has 30 unique dependencies (18 with no dependencies.)
rang::resolve("arrow")
#> resolved: 1 package(s). Unresolved package(s): 0 
#> $`cran::arrow`
#> The latest version of `arrow` [cran] at 2023-08-14 was 12.0.1.1, which has 14 unique dependencies (9 with no dependencies.)

Created on 2023-09-13 with reprex v2.0.2

chainsawriot commented 1 year ago

@schochastics Yes, it is about moving either arrow or readODS from Suggests to Import.

There was a time when all packages were in Imports. However, it would increase the checking time (installing dependencies) and therefore formats that considered of secondary importance were moved to Suggests. And yes, dependency should also be taken into consideration.

chainsawriot commented 1 year ago

I think this is a better comparison: the additional packages that need to be installed by introducing it to Imports. readODS might have a lot of dependencies but most of them are overlapped with the existing packages in Imports. The difference is just one between arrow and readODS. Probably the issue now has more to do with utility.

original_deps <- c("tools", "stats", "utils", "foreign", "haven", "curl", "data.table", "readxl", "tibble", "stringi", "writexl", "lifecycle", "R.utils")

ori <- rang::resolve(original_deps, snapshot_date = Sys.Date())
#> Warning: Some package(s) can't be resolved: cran::tools, cran::stats,
#> cran::utils
nrow(rang:::.generate_installation_order(ori))
#> [1] 38

arrow <- rang::resolve(c(original_deps, "arrow"), snapshot_date = Sys.Date())
#> Warning: Some package(s) can't be resolved: cran::tools, cran::stats,
#> cran::utils
nrow(rang:::.generate_installation_order(arrow))
#> [1] 41

readODS <- rang::resolve(c(original_deps, "readODS"), snapshot_date = Sys.Date())
#> Warning: Some package(s) can't be resolved: cran::tools, cran::stats,
#> cran::utils
nrow(rang:::.generate_installation_order(readODS))
#> [1] 40

Created on 2023-09-13 with reprex v2.0.2

chainsawriot commented 1 year ago

arrow has two advantages

  1. It provides support for both parquet and feather.
  2. setclass can be extended with one more class: arrow_table

The disadvantage is no desktop software support.

readODS has desktop software support: LibreOffice, Excel, and Google Sheets. Arguably more adoption. It also supports two formats: ods and fods. But it has no potential for improving rio. Also, the future of data science is more likely to be on arrow than ods.

schochastics commented 1 year ago

hmm tough decision, but i think my vote is on arrow given its importance for DS.

chainsawriot commented 1 year ago

Let's go with arrow then.

chainsawriot commented 1 year ago

TODOs

chainsawriot commented 1 year ago
wlandau commented 1 year ago

Regarding https://github.com/gesistsa/rio/issues/315#issuecomment-1717290900, another disadvantage of arrow is that it is a really heavy and burdensome package dependency. It takes several minutes to compile, and it has platform-dependent compilation issues such as https://github.com/apache/arrow/issues/30556. On top of that, popular Shiny-related packages like datamods and esquisse depend on rio but do not need arrow.

I noticed rio moved arrow to Imports recently, and this is making it hard for my team to containerize Shiny apps at work. Would it be possible to consider switching arrow back to Suggests? From https://github.com/dreamRs/datamods/blob/6a1331830f397f6fd5fdc742758f6901b690dadc/README.md?plain=1#L83-L84, it looks like rio is fundamental to how datamods works, but the latter only specifically mentions formats like Excel and SPSS.

chainsawriot commented 1 year ago

@wlandau Thank you for the input. Unfortunately, your input came at a very bad time point, where rio 1.0.0 (which is supposed to be a stable release) is already on CRAN.

Of course, I don't want you to have bad experience using rio. What I see is a conflict of visions here: we want to add an open format for computational reproducibility; but in real world usage, people use rio for importing Excel and SPSS.

I believe in Agile and we can make mistakes too. Because you represent the user community to give us feedback, we will listen to it.

I am willing to remove arrow from Imports (although I made a mistake to blog about rio 1.0.0 already). But please give me at least some days to think about how to mitigate the impact of this on-and-off arrow feature. At least we will need some time to make setclass = 'arrow' optional. I promise you I will deliver this to CRAN before Friday.

In these few days, please, if you really don't want rio to have arrow, use remotes::install_version("rio", "0.5.30") until Friday.

chainsawriot commented 1 year ago

@wlandau I just wanted to let you know that rio 1.0.1 is on CRAN with no arrow dependency. Thank you very much again for your feedback.

https://cran.r-project.org/web/packages/rio/

cc @schochastics

wlandau commented 1 year ago

@chainsawriot, thank you very much for accommodating. This change really helps my team develop our infrastructure and tools.

jsonbecker commented 6 months ago

This decision has kind of sat wrong with me for a long time. I get the concern about arrow having a heftier compile time, but parquet support is of growing importance. I wonder if the right move is actually to go the other direction? Removing foreign, haven, readxl, and writexl would cut dependencies from 38 to 17 by my count. Removing data.table and tibble goes down to 9.

If rio can't come "batteries included" for some of the most important binary types because that would be too heavy, perhaps rio shouldn't come "batteries included" where possible to dramatically reduce its footprint. That would make it easier for others to depend on rio.

That said, I think depending on rio is generally a bad decision for package developers. I'm not familiar with either datamods or esquisse, but the vision of rio at the start, and its greatest power, is a single interface, especially for R beginners, that's consistent for reading all manner of data types. It's a convenience wrapper, which feels like a bad choice for a dependency.

So another option might be to work upstream on reverse dependencies that don't seem appropriate to remove their reliance on rio and going the other way and bulking up the default install.

jsonbecker commented 6 months ago

In fact, as of this writing, esquisse does not have a rio dependency:

> tools::package_dependencies(package = "rio", reverse = TRUE)
$rio
 [1] "allMT"               "boxr"                "bruceR"
 [4] "childfree"           "cloudstoR"           "datamods"
 [7] "dataquieR"           "DistPlotter"         "dpmr"
[10] "editData"            "epiCleanr"           "estadistica"
[13] "ExPanDaR"            "framecleaner"        "genogeographer"
[16] "gesisdata"           "heterogen"           "IGoRRR"
[19] "importinegi"         "ISRaD"               "kibior"
[22] "metaConvert"         "mmstat4"             "NormalityAssessment"
[25] "normfluodbf"         "octopus"             "pewdata"
[28] "PRISMA2020"          "psData"              "ropercenter"
[31] "tfrmtbuilder"        "varsExplore"         "welo"
chainsawriot commented 6 months ago

@jsonbecker rio is in the Suggests. Soft dependency, maybe. But we still need to check for it when we run revdepcheck.

Thank you for the feedback. I agree with you that the package was meant to be an easy, unified wrapper for interactive usage. But as things naturally evolved, we also need to adapt to the (new) reality that R package developers also use rio perhaps for the file format detection and the default collection of supported file formats such as excel and spss. It is difficult to judge whether the usage of rio as a dependency is a bad choice. For instance, I would say gesisdata (despite the name, not an official GESIS product) makes sense to use rio because the file download depends on file extension and most files in the GESIS Data archive are SPSS, STATA, and Excel. datamonds is perhaps a similar story.

With this reality, it increases the complexity for adjusting the supported formats in the "Default" and "Suggest" tier. Increasing the default formats is nice (like @schochastics and I did for rio v1.0.0 to support parquet 3d91cd5), but also with "do not need x" concerns like the one from @wlandau [^1] . Decreasing the default formats is surely a breaking change. As I said #307 , we should avoid and prioritize stability [^2].

Having said that, please keep this discourse going. Maybe we can find a good solution to this.

[^1]: A hidden issue is the maintenance cost of making a format the default, e.g. to deal with the CRAN issues when perhaps arrow breaks. Let's assume my time is infinite to ease the discussion.

[^2]: And therefore, I am now prioritizing fixing features implemented in the v0.x series but in a broken state, like the testing and fixing the compression mechanism. Rather than adding new formats.

chainsawriot commented 6 months ago

Just a slight update: To understanding the packages that use rio, we should also look at recursive dependencies. rio is indeed a hard dependency of esquisse, via datamods.

tools::package_dependencies(packages = "rio", reverse = TRUE, recursive = TRUE)
#> $rio
#>  [1] "allMT"               "boxr"                "bruceR"             
#>  [4] "childfree"           "cloudstoR"           "datamods"           
#>  [7] "dataquieR"           "DistPlotter"         "dpmr"               
#> [10] "editData"            "epiCleanr"           "estadistica"        
#> [13] "ExPanDaR"            "framecleaner"        "genogeographer"     
#> [16] "gesisdata"           "heterogen"           "IGoRRR"             
#> [19] "importinegi"         "ISRaD"               "kibior"             
#> [22] "metaConvert"         "mmstat4"             "NormalityAssessment"
#> [25] "normfluodbf"         "octopus"             "pewdata"            
#> [28] "PRISMA2020"          "psData"              "ropercenter"        
#> [31] "tfrmtbuilder"        "varsExplore"         "welo"               
#> [34] "ChineseNames"        "PsychWordVec"        "TestAnaAPP"         
#> [37] "esquisse"            "moreparty"           "safetyGraphics"     
#> [40] "vvdoctor"            "ggplotAssist"        "rrtable"            
#> [43] "SemNetCleaner"       "presenter"           "tidybins"           
#> [46] "validata"            "shinyrecipes"        "FMAT"               
#> [49] "webr"                "scicomptools"

Created on 2024-05-14 with reprex v2.1.0

chainsawriot commented 5 months ago

Keep an eye on this

https://github.com/r-lib/nanoparquet/

chainsawriot commented 5 months ago

According to cransay, nanoparquet has been submitted to CRAN.

chainsawriot commented 5 months ago

nanoparquet is now on CRAN.

https://cran.r-project.org/web//packages/nanoparquet/index.html

Min R version is 4.0.0.

chainsawriot commented 5 months ago

@wlandau I am thinking about adding back parquet support. But this time, I would like to try it with nanoparquet. I tried and the compilation took around 10s. Also, binary package of it is available from P3M.

I don't want to repeat the same thing like v1.0.0, i.e. you only noticed the added arrow only after a new version of rio was on CRAN. I was wondering whether you can try and evaluate the development version with nanoparquet later and provide your feedback? If it is not practical, then I will put it in Suggests, like the last time.

Thank you very much!

cc. @jsonbecker @schochastics

chainsawriot commented 5 months ago

Maybe I should reach out to the datamods team, e.g. @pvictor .

pvictor commented 5 months ago

I'm not sure I understand the problem, is it because datamods (and esquisse) depends on rio?

chainsawriot commented 5 months ago

@pvictor datamods is usually dockerized and datamonds depends on rio. Therefore, every time datamonds users (e.g. @wlandau ) dockerize datamonds, they also have to install rio during the image building phase. Therefore, if rio adds a hard dependency (like previously, arrow), it increases the installation time of datamonds, especially for the ones that require C/C++ compilation on Linux [^1]. And previously, we reverted a decision to add to add arrow in order to support parquet.

Now, with the release of nanoparquet by the Posit Team (https://www.tidyverse.org/blog/2024/06/nanoparquet-0-3-0/), it seems to be possible again to add back the support of parquet, without the compilation problems of arrow. But of course, one dependency more is one more dependency to install. And nanoparquet also requires compilation, although the compilation is very fast. I would like to seek for your view on how it would impact datamonds, and perhaps also the users of datamonds.

  1. Adding one light-weight dependency for the support of parquet by default, would that be useful for the users of datamonds?
  2. It will also boost the R version requirement to 4.0.0, because nanoparquet asks for >=4.0.0. Currently, we check for 3.5 on CI. Would that also be a problem?

Thank you very much!


[^1]: Let's assume one doesn't know how (or is not possible) to use Linux binary installation solutions such as P3M, r2u, or r-universe.

chainsawriot commented 4 months ago

@wlandau @pvictor I just wanted to let you know that I have produced a branch that adds back the default support for parquet using nanoparquet. #444

It would be super nice if you could give it a test and see if it has any impact on your use cases. From my testing, it increases the compiling time by 21 seconds on a blank state Rocker container. I was wondering if this level of increase in compiling is acceptable. At least it is not "several minutes" as mentioned here.

I will consider your comments / evaluations before merging it to main. Thank you very much!

cc @jsonbecker @schochastics

wlandau commented 4 months ago

It's been a while since I looked at this thread, and the stuff I maintain no longer strongly depends on rio. From my perspective, feel free to do whatever works for you. It's encouraging that the compilation time went down so much.

pvictor commented 4 months ago

Great work @chainsawriot , it's great that {rio} support parquet files!

chainsawriot commented 3 months ago

nanoparquet does not support big endian platforms at the moment and it's not on the priority for the developers of nanoparquet r-lib/nanoparquet#21 . It probably won't affect >99% of the users. Some possible affected platforms are 32bit powerpc darwin, as reported here #445 . But we have to take this into consideration.

barracuda156 commented 3 months ago

@chainsawriot To be explicit, of course I do not expect you to fix anything for big-endian in nanoparquet code. It is just unnecessary to break rio for big-endian platforms due to a dependency, which, however desirable, is not essential. It is fine if related functionality will be unavailable on some platforms; if moving nanoparquet to suggests is not an acceptable solution, another way is to offer configure option to disable it (that way default behavior won’t change).

P. S. Otherwise any solution from my side will be ugly: either I need to peg rio to an earlier version and never update it, or I need to patch the code to revert nanoparquet, which is a pain to maintain, and MacPorts folks dislike extra patches, or I need to prohibit it for big-endian archs for no good reason, which is not acceptable for me and will potentially hurt some users, however few.

chainsawriot commented 3 months ago

@barracuda156 Thank you for chipping in. I actually don't mind moving nanoparquet to "Suggests". We did it previously with arrow anyway. Maybe the timing for supporting parquet is not right.

I will give you an update. I really hope that I can finish it before the CRAN summer break.

chainsawriot commented 3 months ago

@barracuda156 I rolled back for now and it should be on CRAN soon. But I really hope that you can help @gaborcsardi to make nanoparquet support big endian platforms because I think you have the expertise.

barracuda156 commented 3 months ago

@barracuda156 I rolled back for now and it should be on CRAN soon. But I really hope that you can help @gaborcsardi to make nanoparquet support big endian platforms because I think you have the expertise.

@chainsawriot Thank you, update merged in https://github.com/macports/macports-ports/pull/25073/commits/97b00a20aedae06f25be68e89cdfdfc270929b33