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 · 17 comments
Open

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

chainsawriot opened this issue Aug 29, 2023 · 17 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

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