TileDB-Inc / TileDB-R

R interface to TileDB: The Modern Database
https://tiledb-inc.github.io/TileDB-R
Other
103 stars 18 forks source link

Build enhancement for Conda #722

Closed eddelbuettel closed 5 months ago

eddelbuettel commented 5 months ago

PR #710 (with SC 47400) brought in a small extension which included small Makevars (the Makefile snippet used by the R package) changes. As was seen in the subsequent release this was not fully Conda-compliant but additional tests helped determine a very small additional fix. A review by @jdblischak (who already tested this) would be nice but I cannot currently request one.

jdblischak commented 5 months ago

I tested this PR on my feedstock fork (commit, build)

I still think we should merge this PR since it unblocks this new feature for the osx-64 conda binary. However, we'll need to do some troubleshooting before the next r-tiledb-feedstock release in order to figure out which test(s) to skip on linux-aarch64. From comparing the R 4.2 and 4.3 logs, it appears the error occurs on the 20th test in test_fragmentinfo.R

jdblischak commented 5 months ago

I suspect the error is happening here. Any reason to suspect this code would be consistently problematic for R 4.3 (but not 4.2) and linux-aarch64 (but not linux-64)? Worst case I'll just add a patch to the conda recipe to skip on linux-aarch64

https://github.com/TileDB-Inc/TileDB-R/blob/e37db85a3bf57fd1577e251f4e97c94aa550bf3b/inst/tinytest/test_fragmentinfo.R#L56-L65

eddelbuettel commented 5 months ago

Two conversations here:

  1. The PR itself which adds build to Conda breaks macOS builds (once that ever-so-helpful decides to unqueue itself and actually run anything). Ball in my court for this, will try to work out with mac-builder or rhub what needs to change.
  2. The aarch64 aka arm64 issue: Strictly no idea, and 'old enough code' and tests to have been subjected to R 4.3.* before. No idea what's going on, no ability to do any work as I do not have access to aarch64 aka arm64 right now.
johnkerl commented 5 months ago

AWS has linux + aarch64 instances available for spin-up ...

eddelbuettel commented 5 months ago

@johnkerl I am aware. I am also aware that certain vendors sell such machines. What I am saying is that I have much easier access to other platforms, and sufficient problems with those.

johnkerl commented 5 months ago

FWIW, on an EC2 linux+aarch64 system, starting up & ssh'ing in & freshly updating to core 2.24 & tiledb-r 0.28 I have

$ Rscript -e 'tinytest::test_package("tiledb")'
test_aggregates.R.............   20 tests OK 0.7s
test_arrayschema.R............   30 tests OK 35ms
test_arrayschemaevolution.R...   64 tests OK 1.9s
test_arrowio.R................   39 tests OK 0.7s
test_attr.R...................   31 tests OK 0.4s
test_config.R.................   23 tests OK 27ms
test_ctx.R....................    8 tests OK 12ms
test_dataframe.R..............   58 tests OK 3.3s
test_datetime.R...............    5 tests OK 0.2s
test_dim.R....................  190 tests OK 2.4s
test_dimsubset.R..............   26 tests OK 10.5s
test_domain.R.................   13 tests OK 15ms
test_filestore.R..............   15 tests OK 0.2s
test_filter.R.................   29 tests OK 19ms [Exited at #99: Skipping remainder on Linux systems without AVX2]
test_filterlist.R.............    4 tests OK 7ms
test_fragmentinfo.R...........   25 tests OK 0.8s
test_group.R..................   76 tests OK 0.5s
test_libtiledb.R..............  181 tests OK 91ms
test_matrix.R.................    0 tests    Error in t(M2) : unused argument (M2)
Calls: <Anonymous> ... expect_equivalent -> fun -> expect_equal -> all.equal
Execution halted
jdblischak commented 5 months ago

FWIW, on an EC2 linux+aarch64 system, starting up & ssh'ing in & freshly updating to core 2.24 & tiledb-r 0.28 I have

Interesting. In your EC2 instance, all 25 of the test_fragmentinfo.R tests passed.

What version of R do you have installed and also what version of {Matrix}?

library("Matrix")
sessionInfo()
johnkerl commented 5 months ago

What version of R do you have installed and also what version of {Matrix}?

Ah, nerts, this instance came with an old R 4.1.2 which I haven't updated:

https://gist.github.com/johnkerl/7e3def5c7717a91e691b887a4df26268

eddelbuettel commented 5 months ago

R may not matter but then again I would not know as I haven't touched R 4.1.* in three years.

A pre-1.7.0 release Matrix package may matter, but if the other packages are all consistently built against the old one it would not matter either.

johnkerl commented 5 months ago

My usual recipe for EC2-instance R upgrades isn't working on this linux+aarch64 system (I don't know why) -- @jdblischak @eddelbuettel please let me know if I'd be adding value here by continuing to debug the upgrade fail ...

https://gist.github.com/johnkerl/651c12e9d9a2500b007c33d2291f7bb3

johnkerl commented 5 months ago

In particular https://gist.github.com/johnkerl/70bb94b472b29099ee0ba9e8be2626cf

ubuntu@larch[prod][][~]$ sudo add-apt-repository "deb https://cloud.r-project.org/bin/linux/ubuntu $(lsb_release -cs)-cran40/"
...
Reading package lists... Done
E: Failed to fetch http://us-east-1.ec2.archive.ubuntu.com/ubuntu/dists/jammy/main/binary-arm64/Packages  404  Not Found [IP: 3.87.126.146 80]
E: Failed to fetch http://us-east-1.ec2.archive.ubuntu.com/ubuntu/dists/jammy-updates/main/binary-arm64/Packages  404  Not Found [IP: 3.87.126.146 80]
E: Failed to fetch http://us-east-1.ec2.archive.ubuntu.com/ubuntu/dists/jammy-backports/main/binary-arm64/Packages  404  Not Found [IP: 3.87.126.146 80]
E: Failed to fetch http://security.ubuntu.com/ubuntu/dists/jammy-security/main/binary-arm64/Packages  404  Not Found [IP: 91.189.91.82 80]
E: Some index files failed to download. They have been ignored, or old ones used instead.

so I'll hold off trying to upgrade R on this linux+aarch64 Ubuntu 22.04 system

eddelbuettel commented 5 months ago

As owner of this issue and PR I took the liberty of marking the very interesting and passionate (but not really relevant here) discussion of another build platform as off-topic.

The current state (and primary puzzle) is that enabling Conda killed macOS. Bad trade-off.

eddelbuettel commented 5 months ago

So we now have the CI Actions back to :heavy_check_mark: but there may be remaining Conda questions.

jdblischak commented 5 months ago

Testing the latest change that uses the env var CONDA_BUILD to configure the setup in the Azure macOS conda builds

https://dev.azure.com/jdblischak/feedstock-builds/_build/results?buildId=863&view=results

eddelbuettel commented 5 months ago

Yay! Looks like we won that battle:

image

The remaining fail is not new, and a continued puzzle. So onwards, gonna merge this now.