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

[r] Controlled downgrade of TileDB-R to ensure matching TileDB Core #1994

Closed
wants to merge 3 commits into from

Conversation

eddelbuettel
Copy link
Contributor

Issue and/or context:

The TileDB-R package builds against TileDB Core, and while we can ensure a minimum version
we cannot ensure an exact version. That can lead to cases as right now where TileDB-R 0.23.0 is
at CRAN with a default of TileDB Core 2.19.0, yet TileDB-SOMA is still set up to build with TileDB
Core 2.18.2. This mismatch is undesirable but a little difficult to avoid for example in continuous
integration where R will always go to the newest package version.

As an experiment, @mojaveazure and I have set up two helper repos. The first ensures we can still build
the previous TileDB-R version at r-universe (which would otherwise pick newest versions from
CRAN and/or GitHub). The second deploys these builds in a particular R repository hosted on
GitHun from which we can install. Together, these provide a simple 'time machine' ability to
the previous binary.

To illustrate, by default we end with these packages (when running on Ubuntu 22.04 as for CI)

  > show_package_versions()
  tiledbsoma:    1.6.0
  tiledb-r:      0.23.0
  tiledb core:   2.19.0
  libtiledbsoma: 2.18.2
  R:             R version 4.3.2 (2023-10-31)
  OS:            Ubuntu 22.04.1 LTS
  >

Whereas once we do a controlled downgrade we get

  > library(tiledbsoma)
  > show_package_versions()
  tiledbsoma:    1.6.0
  tiledb-r:      0.22.0
  tiledb core:   2.18.2
  libtiledbsoma: 2.18.2
  R:             R version 4.3.2 (2023-10-31)
  OS:            Ubuntu 22.04.1 LTS
  >

Changes:

The PR adds a new helper script and invokes it from the r-valgrind action for continuous integration.

Notes for Reviewer:

SC 38681

Copy link

Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Merging #1994 (11b37ac) into main (bdff402) will decrease coverage by 13.11%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1994       +/-   ##
===========================================
- Coverage   89.07%   75.96%   -13.11%     
===========================================
  Files          34      126       +92     
  Lines        3578    10049     +6471     
  Branches        0      161      +161     
===========================================
+ Hits         3187     7634     +4447     
- Misses        391     2340     +1949     
- Partials        0       75       +75     
Flag Coverage Δ
libtiledbsoma 69.86% <ø> (?)
python 89.73% <ø> (+0.65%) ⬆️
r 67.38% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 89.73% <ø> (+0.65%) ⬆️
libtiledbsoma 53.52% <ø> (∅)

@eddelbuettel eddelbuettel force-pushed the de/sc-38681/controlled_tiledb_downgrade branch from a33c505 to ad61711 Compare January 9, 2024 16:39

core_from_tiledb <- tiledb::tiledb_version(compact=TRUE)
core_from_soma <- tiledbsoma:::libtiledbsoma_version(compact=TRUE)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On #1993 I found it helpful to include

cat("core_from_tiledb", as.character(core_from_tiledb), "\n")
cat("core_from_soma", core_from_soma, "\n")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- I developed this in another repo first (to keep things calmer here) and did similar things there.

@eddelbuettel eddelbuettel force-pushed the de/sc-38681/controlled_tiledb_downgrade branch from 37beead to 11b37ac Compare January 9, 2024 19:31
@johnkerl
Copy link
Member

johnkerl commented Apr 2, 2024

Controlled downgrade is effective and in place as a repeatable process. I have had zero problems with this since then in CI, and releases are working well in this regard.

@johnkerl johnkerl closed this Apr 2, 2024
@eddelbuettel
Copy link
Contributor Author

I don't have the energy to debate this endlessly but we had reports of failed installations just days ago. What we set up works if and only if the stars align and tiledb-r is not ahead. I maintain what we can do better.

@johnkerl
Copy link
Member

johnkerl commented Apr 2, 2024

@eddelbuettel these are all for CI, and CI works. When @nguyenv is done with C++-ification and removing our dependency on TileDB-R (and TileDB-Py), this problem will cease to exist.

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Apr 2, 2024

We had reports from users being unable to install. That is not CI.

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

Successfully merging this pull request may close these issues.

2 participants