Follow up to executor example comments #184
Conversation
| @@ -45,16 +45,14 @@ class PriorityExecutor(Executor): | |||
| def __init__(self): | |||
| super().__init__() | |||
| self.high_priority_nodes = set() | |||
| self.low_priority_thread = None | |||
| self.hp_executor = ThreadPoolExecutor(max_workers=10) | |||
There was a problem hiding this comment.
I know it is only an example... but 10 seems a little high. Maybe cpu_count()?
sloretz
added
in review
| @@ -45,16 +46,14 @@ class PriorityExecutor(Executor): | |||
| def __init__(self): | |||
| super().__init__() | |||
| self.high_priority_nodes = set() | |||
| self.low_priority_thread = None | |||
| self.hp_executor = ThreadPoolExecutor(max_workers=multiprocessing.cpu_count()) | |||
There was a problem hiding this comment.
cpu_count() may raise NotImplementedError which you need to catch and fall back to a hard coded value.
There was a problem hiding this comment.
How about omitting max_workers? In python 3.5 it will use os.cpu_count() * 5 if it's implemented, or 5 workers if it's not.
There was a problem hiding this comment.
Sounds like high defaults when not doing IO-intense operations. Since this example doesn't "do" much it probable doesn't matter. Not sure if it sets a good example - maybe with a comment to document what the default is that is ok.
There was a problem hiding this comment.
cpu_count() or 4 workers in 5faf216
| @@ -46,7 +46,7 @@ class PriorityExecutor(Executor): | |||
| def __init__(self): | |||
| super().__init__() | |||
| self.high_priority_nodes = set() | |||
| self.hp_executor = ThreadPoolExecutor(max_workers=multiprocessing.cpu_count()) | |||
| self.hp_executor = ThreadPoolExecutor(max_workers=(os.cpu_count() or 4)) | |||
There was a problem hiding this comment.
Maybe remove the unnecessary parenthesis?
|
Builds passed and approved, squash/merging |
Follow up to comments on #182 that didn't get addressed before r2b3
Simplified custom_executor example
Shortened test paths
Linux
Linux-aarch64
macOS
Windows