-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add desmod.pool.Pool for modeling pool of resources #16
Conversation
286f77d
to
cba4d3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, overall. I have a few comments though.
desmod/pool.py
Outdated
def _trigger_get(self, _=None): | ||
if self._getters and self.level: | ||
for index, get_ev in enumerate(self._getters): | ||
get_ev = self._getters[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why reassign get_ev
? It already exists as a loop variable?
desmod/pool.py
Outdated
get_ev.succeed(item) | ||
if self._get_hook: | ||
self._get_hook() | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The control flow in _trigger_get() is such that it is possible for the getter at the head of the line (_getters
) can be starved. This can happen if the first getter has a large amount
and subsequent getters have small amount
s. The issue is that the _getters
is not treated in a FIFO manner. This is inconsistent with simpy.Container
which always fulfills the first getter before fulfilling any subsequent getters.
Another difference with simpy.Container
is that _trigger_get()
only fulfills one getter instead of fulfilling getters (in FIFO order) until level
is below the next getter's amount
.
I suggest that we want Pool
's semantics to be aligned with simpy.Container
unless there is a compelling reason I am unaware of.
desmod/pool.py
Outdated
put_ev = self._putters.pop(0) | ||
put_ev.succeed() | ||
self._add_items(put_ev.amount) | ||
self._trigger_when_new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The when_new
waiters will be triggered unconditionally even if the putter's amount
is zero. Although somewhat pathological for a putter to have amount=0
, it might be best to nonetheless protect _trigger_when_new()
with a test of level
.
desmod/pool.py
Outdated
"""Simulation pool of arbitrary items. | ||
|
||
`Pool` is similar to :class:`simpy.resources.Container`. | ||
It provides a simulation-aware container for managing a pool of objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, simpy.Container
and thus Pool
do not necessarily model discrete objects. An important use case is to model continuous quantities (i.e. with a float
level). This doc string should be clarified.
desmod/pool.py
Outdated
@property | ||
def size(self): | ||
"""Number of items in pool.""" | ||
return self.level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear why we need/want this size
alias for level
.
desmod/probe.py
Outdated
@@ -124,3 +130,19 @@ def hook(): | |||
callback(queue.remaining) | |||
|
|||
queue._put_hook = queue._get_hook = hook | |||
|
|||
|
|||
def _attach_pool_size(pool, callbacks): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming: _attach_pool_level()
?
desmod/pool.py
Outdated
|
||
def _remove_items(self, item_count=1): | ||
self.level -= item_count | ||
return item_count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These _add_items()
and _remove_items()
methods only have one call site each and do a trivial operation. I'd suggest moving these level
increments and decrements inline.
075d9a2
to
43d035a
Compare
Pool offers a similar behavior to simpy.Container with some additional events: * Pool.when_any() when the pool is non-empty * Pool.when_new() when items are inserted in pool * Pool.when_full() when the pool is full Pool is to simpy.Container what Queue is to simpy.Store
* Fix a bug in when testing when_new event * Add case when a consumer needs to wait a producer
desmod/pool.py
Outdated
"Amount {} greater than pool's {} capacity {}".format( | ||
get_ev.amount, str(self.name), self.capacity)) | ||
if get_ev.amount <= self.level: | ||
self._getters.remove(get_ev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The control flow here is suspicious since we're removing from the container we're iterating (_getters
). I think we need a while
loop instead of a for
loop. It might be helpful to reference simpy's BaseResource._trigger_put()
which uses a while
loop for, I believe, this very reason.
desmod/pool.py
Outdated
""" | ||
|
||
def __init__(self, env, capacity=float('inf'), hard_cap=False, | ||
init_level=0, name=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest renaming init_level
to init
for compatibility with simpy.Container
.
For the same reason, I also suggest the parameter order be (env, capacity, init, hard_cap, name)
so that Container
's specifying init
positionally can be trivially replaced with Pool
.
* Fix unsafe loop in Pool._trigger_get * Add check that Pool.put and Pool.get take only positive values * Add tests to validate correct handling of Pool.put(0) and Pool.get(0)
Thanks @vagvaz! |
Pool offers a similar behavior to simpy.Container with some
additional events:
Pool is to simpy.Container what Queue is to simpy.Store