Have option to set thread count to local maximum #1716

brenthuisman · 2021-10-14T14:58:23Z

Pyarb specific.
proc_allocation_shim() throws error if user sets threads to zero.
arbor.context accepts avail_threads, which sets the number of threads to arbenv::thread_concurrency()
- This introduces a dependency on arbenv for Pyarb.
Associated docs and tests added.

halfflat · 2021-10-14T15:31:15Z

I'd really like this done according to the outline in #988, with the heuristics in the arborenv lib.

thorstenhater

I like the newtype pattern here, but as alternative consider a (set of) static function(s)

std::option<unsigned> thread_count_avail_cores() {...}
std::option<unsigned> thread_count_from_omp() {...}
...

maybe that's even cleaner.

arbor/include/arbor/context.hpp

arbor/include/arbor/arbexcept.hpp

noraabiakar · 2021-10-15T15:03:05Z

arbor/include/arbor/context.hpp

@@ -13,6 +16,32 @@ struct dry_run_info {
            num_cells_per_rank(cells_per_rank) {}
 };

+struct thread_count {


This class is mixing up arbor and arborenv functionailty. I don't think this is the right place for it.

Users interact with these classes directly, so sanitizing user input (0 is not a good thread number) in this place makes sense.

Or do you mean to just move the struct to concurrency.cpp? It would make arborenv a dependency of the Python package though.

noraabiakar · 2021-10-15T15:04:31Z

arbor/include/arbor/context.hpp

+    }
+
+    static thread_count avail_threads() { 
+        auto tc = thread_count{std::thread::hardware_concurrency()};


Why doesn't this just return an unsigned?

It does; unsigned is the return type of std::thread::hardware_concurrency().

…e 0 check

brenthuisman · 2021-10-20T08:52:19Z

@thorstenhater Shall I use thread_concurrency() and add arbenv to the Python packages requirement? (see #1725)

brenthuisman · 2021-12-07T09:54:14Z

Yes, I shall.

…ption from C++, update test and doc

noraabiakar

Looks good! I just have a few suggestions.

arbor/include/arbor/context.hpp

noraabiakar · 2022-01-19T15:05:54Z

arbor/arbexcept.cpp

@@ -37,6 +37,11 @@ bad_global_property::bad_global_property(cell_kind kind):
    kind(kind)
 {}

+zero_thread_requested_error::zero_thread_requested_error(unsigned nbt):
+    arbor_exception(pprintf("threads must be a positive integer")),


Since this PR seems to be restricted to pyarb changes, I think this exception would work better as a pyarb_error (defined in python/error.hpp)

I see your point, but I'm not entirely happy with 1. this split anyway and 2. the particular contents of python/error.hpp. First of all, python/pyarb.cpp already contained one error translator file_not_found_error, which is a much clearer pattern: you register errors raised in C++ code, and translate them as you wish. Since the code of this particular change is C++ (and really all our code), even though restricted to being raised within Pyarb for the moment, it makes more sense to me to follow the same pattern. The pyarb_error in python/error.hpp should really be removed, it relies on implicit translation by pybind11 and it's uses broken up into more meaningful exception classes.

If you agree, I can do that in a follow up PR. What do you think?

The use of translation was a test and especially aimed at FileNotFound, because users expect FNF instead of RuntimeError. But yes, now that this is in for a while and caused no issues, we should port all our errors.

I see what you're saying, but I don't think the solution is to move all of our custom errors to the arbor library. The errors that are used only in the pyarb library should be defined there, either in python/error.hpp or somewhere else.
I agree that we should translate our exceptions into python exceptions whenever possible, but that has nothing to do with where the exceptions are defined.
I think it's better to follow the current convention of using pyarb_error instead of arbor_exception in this case.

There are already custom Arbor errors in arbor/arbexcept.*pp , right? They are not specific to a part of the API, else they would not live in one file. I don't think Python needs to be an exception (ha), other parts of Arbor may very well raise a zero_thread_requested_error at some point. I thought that having all errors in one place is so you can quickly find out if the exception you are raising already fits an existing class.

Now, if the exception was very specific to Python I would agree with you, but this is about user input, which is not specific to Python.

I prefer to start (well, continue!) writing new exceptions in this pattern, and tackle existing pyarb_errors in the near future.

We're looking at 2 libraries

arbor is the core arbor C++ library and has it's own exceptions in arbor/arbexcept.*pp.

pyarb is the python wrapper library, it needs arbor as a dependency but they are separate libraries. It has it's own exceptions in python/error.*pp.

An error that is only used in pyarb doesn't need to live in arbor.
Separating the errors keeps the code clear, you can look at the type of the error if it's pyarb_error you know it's been raised from pyarb, if it's arbor_exception it's been raised from arbor.
There's no need to accumulate all errors in the same file in the arbor lib. We can replicate what we have in the arbor lib in the pyarb lib as long as we keep that separation between the libraries clear.

It is not a Python error, but an input error. It just happens to be only raised by the Python binding for the moment. In general, I think they should overlap as much as possible, not as little as possible.

I think "Python errors" should be seen as exceptions raised by Python machinery, e.g. C++ is unlikely to ever need a TabError.

I think this is a small issue and I don't want to keep going around in circles. The current setup is to use pyarb_error in the pyarb library and I think this error belong there. It's up to you at the end of the day.

python/context.cpp

test/unit/test_local_context.cpp

python/test/unit/test_contexts.py

python/context.cpp

doc/python/hardware.rst

thorstenhater

👍

noraabiakar · 2022-01-24T11:05:09Z

bors try

bors · 2022-01-24T11:25:58Z

try

Build succeeded:

ci/gitlab/gitlab.com

* Pyarb specific. * `proc_allocation_shim()` throws error if user sets threads to zero. * `arbor.context` constructor accepts `threads` set to `"avail_threads"`, which sets the number of threads to `arbenv::thread_concurrency()` * This introduces a dependency on arbenv for Pyarb. * Docs and tests added. Fixes arbor-sim#1692

brenthuisman added 5 commits October 12, 2021 17:25

threads class

e5ac3c2

make unsigned, catch negative numbers

0a32ed3

Merge remote-tracking branch 'upstream/master' into feat/moar-cores

1ac7522

add namespace, declare python error

686c315

Python constructors, tests

151b330

brenthuisman requested a review from thorstenhater October 14, 2021 14:58

brenthuisman changed the title ~~Feat/moar cores~~ Have option to set thread count to local maximum Oct 14, 2021

Merge remote-tracking branch 'upstream/master' into feat/moar-cores

7b02e31

pybind: back to the future

35fa9ce

thorstenhater requested changes Oct 14, 2021

View reviewed changes

brenthuisman added 4 commits October 15, 2021 12:08

add documentation

dbda485

pybind back to same as master

e70813a

review

407c56e

Fix check

eba9e82

noraabiakar reviewed Oct 15, 2021

View reviewed changes

use thread_count in pythons proc_allocation_shim in order to reuse th…

5020301

…e 0 check

brenthuisman marked this pull request as ready for review October 15, 2021 15:30

brenthuisman requested a review from thorstenhater October 15, 2021 15:34

akuesters added this to the v0.6 milestone Nov 24, 2021

brenthuisman added 6 commits January 18, 2022 16:04

merge with master

c1f7270

fix problems

49f4853

deduplicate arbenv::concurrency and thread_count struct, remove the o…

00d736a

…ption from C++, update test and doc

forgot to add

0799432

forgot to add 2

fbaed17

forgot an include

2ef44e3

noraabiakar suggested changes Jan 19, 2022

View reviewed changes

brenthuisman requested a review from noraabiakar January 20, 2022 12:24

brenthuisman added 2 commits January 20, 2022 13:25

nora review

b151a7d

Correction of Pythons context and proc_allocation documentation

baf3113

brenthuisman mentioned this pull request Jan 24, 2022

Reorganize exceptions: Python #1812

Closed

merge master

e0c6c32

thorstenhater approved these changes Jan 24, 2022

View reviewed changes

bors bot added a commit that referenced this pull request Jan 24, 2022

Try #1716:

b3f91ca

noraabiakar approved these changes Jan 24, 2022

View reviewed changes

brenthuisman merged commit 22bd848 into arbor-sim:master Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have option to set thread count to local maximum #1716

Have option to set thread count to local maximum #1716

brenthuisman commented Oct 14, 2021 •

edited

Loading

halfflat commented Oct 14, 2021

thorstenhater left a comment

noraabiakar Oct 15, 2021

brenthuisman Oct 15, 2021 •

edited

Loading

noraabiakar Oct 15, 2021

brenthuisman Oct 15, 2021

brenthuisman commented Oct 20, 2021

brenthuisman commented Dec 7, 2021

noraabiakar left a comment

noraabiakar Jan 19, 2022

brenthuisman Jan 20, 2022

thorstenhater Jan 20, 2022

noraabiakar Jan 20, 2022

brenthuisman Jan 21, 2022

noraabiakar Jan 21, 2022

brenthuisman Jan 21, 2022

noraabiakar Jan 21, 2022 •

edited

Loading

thorstenhater left a comment

noraabiakar commented Jan 24, 2022

bors bot commented Jan 24, 2022

Have option to set thread count to local maximum #1716

Have option to set thread count to local maximum #1716

Conversation

brenthuisman commented Oct 14, 2021 • edited Loading

halfflat commented Oct 14, 2021

thorstenhater left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brenthuisman Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brenthuisman commented Oct 20, 2021

brenthuisman commented Dec 7, 2021

noraabiakar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noraabiakar Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

thorstenhater left a comment

Choose a reason for hiding this comment

noraabiakar commented Jan 24, 2022

bors bot commented Jan 24, 2022

try

brenthuisman commented Oct 14, 2021 •

edited

Loading

brenthuisman Oct 15, 2021 •

edited

Loading

noraabiakar Jan 21, 2022 •

edited

Loading