Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

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

Open
chainsawriot opened this issue Aug 29, 2023 · 32 comments
Open

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

chainsawriot opened this issue Aug 29, 2023 · 32 comments

Comments

@chainsawriot
Copy link
Collaborator

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 chainsawriot pinned this issue Sep 3, 2023
@chainsawriot
Copy link
Collaborator Author

#340

@chainsawriot chainsawriot unpinned this issue Sep 6, 2023
@schochastics
Copy link
Member

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

chainsawriot commented Sep 13, 2023

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
Copy link
Member

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

@chainsawriot
Copy link
Collaborator Author

Let's go with arrow then.

@chainsawriot chainsawriot changed the title Support at least one binary open standard out of the box Move arrow to Imports Sep 13, 2023
@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Sep 13, 2023

TODOs

  • Move arrow to Imports
  • ? add arrow_table as a possible class?

chainsawriot added a commit that referenced this issue Sep 13, 2023
@chainsawriot chainsawriot reopened this Sep 13, 2023
@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Sep 13, 2023

  • Update Internal data

chainsawriot added a commit that referenced this issue Sep 13, 2023
@wlandau
Copy link

wlandau commented Sep 18, 2023

Regarding #315 (comment), 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 apache/arrow#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
Copy link
Collaborator Author

@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 chainsawriot reopened this Sep 18, 2023
@chainsawriot chainsawriot changed the title Move arrow to Imports Support at least one binary open standard out of the box Sep 18, 2023
@chainsawriot chainsawriot removed the v1.0 label Sep 18, 2023
@chainsawriot chainsawriot pinned this issue Sep 19, 2023
@chainsawriot
Copy link
Collaborator Author

@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
Copy link

wlandau commented Sep 19, 2023

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

@jsonbecker
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

@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.

Footnotes

  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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Keep an eye on this

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

@chainsawriot
Copy link
Collaborator Author

According to cransay, nanoparquet has been submitted to CRAN.

@chainsawriot
Copy link
Collaborator Author

nanoparquet is now on CRAN.

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

Min R version is 4.0.0.

@chainsawriot
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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

@pvictor
Copy link

pvictor commented Jun 19, 2024

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

@chainsawriot
Copy link
Collaborator Author

@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!


Footnotes

  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
Copy link
Collaborator Author

@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
Copy link

wlandau commented Jul 16, 2024

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
Copy link

pvictor commented Jul 17, 2024

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

@chainsawriot
Copy link
Collaborator Author

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
Copy link

barracuda156 commented Jul 26, 2024

@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
Copy link
Collaborator Author

@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 added a commit that referenced this issue Jul 26, 2024
chainsawriot added a commit that referenced this issue Jul 26, 2024
* Rollback nanoparquet ref #315

* Bump ver
@chainsawriot
Copy link
Collaborator Author

chainsawriot commented Jul 26, 2024

@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
Copy link

@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 macports/macports-ports@97b00a2

@chainsawriot chainsawriot unpinned this issue Aug 29, 2024
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

6 participants