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/ci] Point at correct TileDB-R location #2275

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Mar 14, 2024

See comments in the affected CI YAML file.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Merging #2275 (bd5bfd9) into main (0fcce69) will decrease coverage by 1.40%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2275      +/-   ##
==========================================
- Coverage   80.28%   78.89%   -1.40%     
==========================================
  Files          87      139      +52     
  Lines        6367    10718    +4351     
  Branches      214      215       +1     
==========================================
+ Hits         5112     8456    +3344     
- Misses       1169     2164     +995     
- Partials       86       98      +12     
Flag Coverage Δ
libtiledbsoma 67.72% <ø> (+3.73%) ⬆️
python 90.99% <ø> (ø)
r 74.69% <ø> (?)

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

Components Coverage Δ
python_api 90.99% <ø> (ø)
libtiledbsoma 48.85% <ø> (+0.15%) ⬆️

@johnkerl
Copy link
Member Author

[sc-43172]

Copy link

This pull request has been linked to Shortcut Story #43172: Update projects to TileDB 2.21.

@johnkerl johnkerl requested a review from eddelbuettel March 14, 2024 19:06
@johnkerl johnkerl marked this pull request as ready for review March 14, 2024 19:06
# sources --- but that would require installing all build dependencies. To see what the most
# recent binary is run e.g. docker run --rm -ti rocker/r2u bash -c 'apt update -qq &&
# apt-cache show r-cran-tiledb' Using the r-universe builds (as below) is a suitable fallback
# as they update more frequently than CRAN.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure I agree with the wording. Sadly I cannot quite put my finger on something better.

But riffing for a moment we seem to have the three situations here:

  • tiledb-r and tiledbsoma agree on versions, the world is good
  • tiledb-r is
    • at release at CRAN (which is what we generally want)
    • at release-candidate in the GH repo and available via selected r-universe builds

That last bit is "unusual but common" and I think the bit we have here today.

And just how he have / had the existing ("sleeping") PR #1994 we have this now. Part of me thinks we may want to discuss how to drive this better from one top-level (possibly outside the repo?) 'declarative' file.

It's tricky.

Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Can you remove the new leading paragraph? I do not think it illuminates sufficiently.

What we have here is that sometimes we need jump a step ahead and sometimes we need to not do that. So we comment these stanza 'in' and 'out' as they give us the binaries we otherwise want to speed things up.

@johnkerl
Copy link
Member Author

johnkerl commented Mar 14, 2024

Can you remove the new leading paragraph? I do not think it illuminates sufficiently.

@eddelbuettel I put it in a while back because I found it to be necessary information for me. And I do release management for this project -- so I believed that was important.

Then, on another commit (not by me), that paragraph disappeared.

Then, on this commit I'm trying to put it back in.

Here, you're asking me to take it back out again.

What I will do is to take the wording I find crucial -- to avoid CI breakages like the one we're having right now -- and I will put that wording at https://github.com/single-cell-data/TileDB-SOMA/wiki/Branches-and-releases, and I will add that hyperlink as a comment within the r-ci.yaml file on this PR, and I will ask you to not remove this crucial hyperlink on future commits.

bd5bfd9

@johnkerl johnkerl requested a review from eddelbuettel March 14, 2024 21:40
@eddelbuettel
Copy link
Contributor

eddelbuettel commented Mar 14, 2024

@johnkerl My comments in this ticket were aimed at illustrating the complexity of the issue. The comment in the yaml did not reflect it to my reading, but focussed on a different (and here less relevant) issue. No more no less. It is by our chosing that we sometimes want what r-universe offers, and sometimes we don't. I maintain that a more central switch may help us all do our job and more easily.

The key aspect remain that we sometime do, and sometimes do not, need these two chunks. Right now all we need was to actually comment the other way to get to prior release of tiledb-r with the (pinned as fallback) tiledb core version used here. That toggle will always have be moved forward or backwards depending on where the SOMA release is relative to the tiledb-r release.

But that has actually little to nothing to with where we get binaries from to speed up the CI. It really is just a complicated form of pinning of tiledb core. As such I found the comment misleading.

@johnkerl johnkerl merged commit 2d230e5 into main Mar 14, 2024
15 checks passed
@johnkerl johnkerl deleted the kerl/avoid-tiledb-r-with-core-2.21 branch March 14, 2024 22:18
github-actions bot pushed a commit that referenced this pull request Mar 14, 2024
* [r/ci] Point at correct TileDB-R location

* Restore valuable comment [skip-ci]

* code-review feedback
johnkerl added a commit that referenced this pull request Mar 14, 2024
* [r/ci] Point at correct TileDB-R location

* Restore valuable comment [skip-ci]

* code-review feedback

Co-authored-by: John Kerl <[email protected]>
johnkerl added a commit that referenced this pull request Apr 5, 2024
@johnkerl johnkerl mentioned this pull request Apr 5, 2024
johnkerl added a commit that referenced this pull request Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants