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

[c++] Use core 2.21.2 [pending TileDB-Py 0.27.2] #2424

Closed
wants to merge 1 commit into from
Closed

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Apr 9, 2024

Issue and/or context: 2.21.2 is out and ready to use.

Changes:

Notes for Reviewer:

This is part of our established procedure.

As noted below in this PR: paused until TileDB-Py has a bump to depend on core 2.21.2.

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Merging #2424 (90c46d6) into main (0fc3cc0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2424   +/-   ##
=======================================
  Coverage   90.61%   90.61%           
=======================================
  Files          37       37           
  Lines        3900     3900           
=======================================
  Hits         3534     3534           
  Misses        366      366           
Flag Coverage Δ
python 90.61% <ø> (ø)

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

Components Coverage Δ
python_api 90.61% <ø> (ø)
libtiledbsoma ∅ <ø> (∅)

@johnkerl johnkerl marked this pull request as ready for review April 9, 2024 18:17
@johnkerl johnkerl changed the title [c++] Use core 2.21.2 [WIP] [c++] Use core 2.21.2 Apr 9, 2024
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.

Potential problem with #2410 as we get tiledb-r with 2.21.1 from CRAN so we may need to relax the comparison and/or provide a tiledb-r with 2.21.2 (which is not that immediately available as the main branch jumped from 2.21.1 to 2.22.0-rc*). One possible solution is to use the same r-universe we use to build tiledbsoma to provide a tiledb-r that matches.

@johnkerl
Copy link
Member Author

johnkerl commented Apr 9, 2024

[deleted comment since I was commenting on the wrong PR]

@eddelbuettel
Copy link
Contributor

@johnkerl What do you think about comparing 'a.b' out of 'a.b.c' for equality? It is the minor we increment with Core releases, and it is that part that bites us. Methinks we were too eager and I need to adjust #2410 with a follow-up. Thoughts?

@johnkerl
Copy link
Member Author

johnkerl commented Apr 9, 2024

@johnkerl What do you think about comparing 'a.b' out of 'a.b.c' for equality? It is the minor we increment with Core releases, and it is that part that bites us. Methinks we were too eager and I need to adjust #2410 with a follow-up. Thoughts?

Different library versions (even micros) can and do cause problems. It's a build error if this happens. We need to enforce that.

@eddelbuettel
Copy link
Contributor

eddelbuettel commented Apr 9, 2024

We need to enforce that.

"Yes but ..." If you enforce that then you just got yourself a new (recurring) task of ensuring such tiledb-r versions are built. This is neither a goal nor target of tiledb-r. Happy to tag releases at GH but if you want lockstep releases (that are very much out if sync with coarser 'major only' CRAN releases) you have to ensure you have them. It's very doable via r-universe and you (as 'consumer of the builds' via SOMA) can drive and schedule their 'production'. And as you say, once the C++-ification finishes it won't be an issue.

Alternatively we can also revert and undo #2410.

@johnkerl
Copy link
Member Author

johnkerl commented Apr 9, 2024

@eddelbuettel thank you and I'll take that risk, and I'll revert #2410 only when and if that becomes an unavoidable Plan B (or C, etc.). I will not make undefined behavior a Plan A -- and having two differing copies of core in memory is very much undefined behavior.

@johnkerl
Copy link
Member Author

johnkerl commented Apr 9, 2024

Note
https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688
shows us finding correct core versions:

Run Rscript -e 'tiledbsoma::show_package_versions()'
tiledbsoma:    1.9.99.3
tiledb-r:      0.25.0
tiledb core:   2.21.2
libtiledbsoma: 2.21.2
R:             R version [4](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:9:5).3.3 (2024-02-29)
OS:            Ubuntu 22.04.4 LTS

These are compatible with one another: 2.21.2 == 2.21.2.

Meanwhile on the Python side (same job) we have

Run python -c 'import tiledbsoma; tiledbsoma.show_package_versions()'
  python -c 'import tiledbsoma; tiledbsoma.show_package_versions()'
  python scripts/show-versions.py
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.11.9/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.9/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.9/x64
    Python[2](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:12:2)_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.9/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/[3](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:12:3).11.9/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.9/x6[4](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:12:4)/lib
tiledbsoma.__version__        1.[5](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:12:5).0rc0.post293.dev101001[6](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:12:6)52
TileDB-Py tiledb.version()    (0, 27, 1)
TileDB core version           2.21.1
libtiledbsoma version()       libtiledb=2.21.2
python version                3.11.9.final.0
OS version                    Linux 6.5.0-101[7](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:12:7)-azure
tiledbsoma.__version__    1.5.0rc0.post293.dev101001652
tiledb.version()          0.27.1
core version              2.21.1
anndata.__version__  (ad) 0.10.7
numpy.__version__    (np) 1.26.4
pandas.__version__   (pd) 2.2.1
pyarrow.__version__  (pa) 15.0.2
scanpy.__version__   (sc) 1.10.1
scipy.__version__    (sp) 1.13.0
python__version__         3.11.[9](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/8619791560/job/23625231688#step:12:9)

which are also compatible with one another -- 2.21.1 == 2.21.1 -- but not at 2.21.2.

Therefore I'll pause this PR until there is a TileDB-Py bump up to depending on 2.21.2. cc @ihnorton @nguyenv .

@johnkerl johnkerl changed the title [c++] Use core 2.21.2 [c++] Use core 2.21.2 [paused] Apr 9, 2024
@johnkerl johnkerl requested a review from Shelnutt2 April 9, 2024 19:29
@johnkerl johnkerl marked this pull request as draft April 9, 2024 19:29
@eddelbuettel
Copy link
Contributor

@johnkerl Awesome find. I live under 2.23.0 but had somehow (wrongly convinced myself we'd get 2.21.1 not .2. So disregard my concern (at least for now).

@johnkerl johnkerl changed the title [c++] Use core 2.21.2 [paused] [c++] Use core 2.21.2 [pending TileDB-Py 0.27.2] Apr 9, 2024
@johnkerl
Copy link
Member Author

It looks like we'll have a tiledbsoma 1.10 with core 2.20 soon, so, I'll abandon this.

@johnkerl johnkerl closed this Apr 12, 2024
@johnkerl johnkerl deleted the kerl/2.21.2 branch April 12, 2024 18:52
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