[c++] Refine concurrency configuration (`soma.compute_concurrency_level`) by bkmartinjr · Pull Request #2978 · single-cell-data/TileDB-SOMA

bkmartinjr · 2024-09-11T17:17:09Z

Issue and/or context:

Introduces direct control over the native thread pool concurrency level in SOMA. Previously, it was indirectly controlled by the TileDB Core sm.compute_currency_control configuration, and would create a thread pool that was 1/2 the size of that value.

Changes:

This PR introduces a new context configuration setting: soma.compute_currency_level, which defaults to the host CPU count. It can be set with an numeric value,eg., in Python:

context = tiledbsoma.SOMATileDBContext(tiledb_config={"soma.compute_currency_level": "20"})

Notes for Reviewer:

sc-54891

the PR also adds:

improved error handling for malformed config values
bound the value between 1 and 1024

codecov · 2024-09-11T17:27:56Z

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.23%. Comparing base (0cfb1bd) to head (e253b1a).
Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2978       +/-   ##
===========================================
- Coverage   89.73%   61.23%   -28.50%     
===========================================
  Files          39       94       +55     
  Lines        4080    12530     +8450     
  Branches        0      786      +786     
===========================================
+ Hits         3661     7673     +4012     
- Misses        419     4657     +4238     
- Partials        0      200      +200

Flag	Coverage Δ
libtiledbsoma	`47.40% <9.09%> (?)`
python	`89.87% <ø> (+0.14%)`	⬆️

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

Components	Coverage Δ
python_api	`89.87% <ø> (+0.14%)`	⬆️
libtiledbsoma	`50.01% <9.09%> (∅)`

johnkerl · 2024-09-11T18:34:48Z

    import numpy as np

-    with pytest.raises(tiledbsoma.SOMAError):
+    with pytest.raises((tiledbsoma.SOMAError, RuntimeError)):


I'll help you fix this -- already done on #2963

TileDB-SOMA/apis/python/src/tiledbsoma/pytiledbsoma.cc

Lines 36 to 45 in c08b5cf

* We're aware of

* https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html

* -- we find empirically that despite this translator, we still

* find it necessary to do explicit catch-and-rethrow within our

* pybind11 functions. See also

* https://github.com/single-cell-data/TileDB-SOMA/pull/2963

*/

py::register_exception_translator([](std::exception_ptr p) {

auto tiledb_soma_error = (py::object)py::module::import("tiledbsoma")

.attr("SOMAError");

TileDB-SOMA/apis/python/src/tiledbsoma/soma_array.cc

Lines 348 to 352 in c08b5cf

try {

array.set_dim_points(dim, points);

} catch (const std::exception& e) {

throw TileDBSOMAError(e.what());

}

(one example callsite)

Also
https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html

Broader issue - filed as sc-54978

johnkerl

LGTM but I do defer to @nguyenv

nguyenv · 2024-09-12T16:58:43Z

+
+
+def test_malformed_concurrency_config_value():
+    import numpy as np


Why import in the test instead of up top?

Just trying to isolate test dependencies. But not a major issue either way....

bkmartinjr added 3 commits September 11, 2024 10:04

refine SOMA compute currency level config handing

ad9c900

add test for config error handling

b927d3c

max bounds on concurrency config value

9368d11

bkmartinjr added 3 commits September 11, 2024 10:28

format lint

a445a0d

run clang-format

3ed922a

format with correct clang-format version

b3721f2

johnkerl changed the title ~~Refine concurrency configuration (soma.compute_concurrency_level)~~ [c++] Refine concurrency configuration (soma.compute_concurrency_level) Sep 11, 2024

test workaround on macos

8338728

johnkerl reviewed Sep 11, 2024

View reviewed changes

bkmartinjr added 5 commits September 11, 2024 11:38

Merge branch 'main' into bkmartinjr/54891-concurrency-config

47ee64e

debugging MacOS error type

c57e17a

debugging MacOS error type - more

12a984f

test lint

c00fc47

final workaround

7db9bb1

bkmartinjr marked this pull request as ready for review September 11, 2024 20:22

bkmartinjr requested review from johnkerl and nguyenv September 11, 2024 20:22

johnkerl mentioned this pull request Sep 11, 2024

[c++/python] More exception-mapping fixes #2980

Merged

bkmartinjr added 2 commits September 11, 2024 16:31

Merge branch 'main' into bkmartinjr/54891-concurrency-config

ddbdc96

remove macos work-around

e253b1a

johnkerl approved these changes Sep 12, 2024

View reviewed changes

nguyenv reviewed Sep 12, 2024

View reviewed changes

nguyenv approved these changes Sep 12, 2024

View reviewed changes

bkmartinjr merged commit 16c9dff into main Sep 12, 2024

bkmartinjr deleted the bkmartinjr/54891-concurrency-config branch September 12, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c++] Refine concurrency configuration (`soma.compute_concurrency_level`)#2978

[c++] Refine concurrency configuration (`soma.compute_concurrency_level`)#2978
bkmartinjr merged 14 commits intomainfrom
bkmartinjr/54891-concurrency-config

bkmartinjr commented Sep 11, 2024 •

edited

Loading

Uh oh!

codecov Bot commented Sep 11, 2024 •

edited

Loading

Uh oh!

johnkerl Sep 11, 2024

Uh oh!

johnkerl Sep 11, 2024

Uh oh!

bkmartinjr Sep 11, 2024

Uh oh!

johnkerl left a comment

Uh oh!

nguyenv Sep 12, 2024

Uh oh!

bkmartinjr Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	* We're aware of
	* https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html
	* -- we find empirically that despite this translator, we still
	* find it necessary to do explicit catch-and-rethrow within our
	* pybind11 functions. See also
	* https://github.com/single-cell-data/TileDB-SOMA/pull/2963
	*/
	py::register_exception_translator([](std::exception_ptr p) {
	auto tiledb_soma_error = (py::object)py::module::import("tiledbsoma")
	.attr("SOMAError");

	try {
	array.set_dim_points(dim, points);
	} catch (const std::exception& e) {
	throw TileDBSOMAError(e.what());
	}



		def test_malformed_concurrency_config_value():
		import numpy as np

Conversation

bkmartinjr commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

johnkerl Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

johnkerl Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

bkmartinjr Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

johnkerl left a comment

Choose a reason for hiding this comment

Uh oh!

nguyenv Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

bkmartinjr Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bkmartinjr commented Sep 11, 2024 •

edited

Loading

codecov Bot commented Sep 11, 2024 •

edited

Loading