Repository navigation
Define limit in populate as number of keys to compute rathers than keys to check #375
Description
At the moment if the function populate is called with the arguments limit=n and reserve_jobs=True, limit sets the number of keys that is checked against the job table. This means if populate is called on two instances with populate(reserve_jobs=True, limit=1), the first instance selects the first key and populates it while the second instance selects the same key, detects that the job is already reserved and stops without populating an entry. The desired behavior from my point of view would be to select the first free key and populate that entry.
This is important when populating tables on a cluster where job runtimes have to be limited and estimated in advance so that one wants to limit the number of keys populated per job.
Activity
ixcat commented on Oct 18, 2017
Any reason not to have both methods?
I could see controlling wasted number-of-failures as well as controlling number-of-computations:
To compute up to 100 items but quitting if there are more than 5 instances of contention, indirectly indicating that there are 'enough' jobs running. That said this is probably not the best way to measure contention and something more robust would take more changes.
Any insight on how the current implementation came to be? Was there a use case or is the current behavior a bug vs the 'correct' meaning described by the ticket submission?
dimitri-yatsenko commented on Oct 18, 2017
Currently, the limit argument is simply passed to the key source query. This is efficient because a limited query may be faster than an unlimited one. Limiting the number of jobs performed would require checking out more and then counting the ones actually computed. As @ixcat suggested, perhaps we should have two different arguments. limit should limit the key_source query as it does now and another argument should limit the number of jobs performed.
E.g.
max_computations only makes sense when smaller than limit.
eywalker commented on Oct 18, 2017
I like the idea of keeping limit and adding a new argument for the new behavior, also to preserve any backward compatibility. Use of limit roughly ensure fixed number of key computed regardless of the number of workers. As @ixcat and @dimitri-yatsenko proposed, let's add new argument like max_computed to count up and limit how many keys were computed by each process.
dimitri-yatsenko commented on Oct 18, 2017
since we use limit without describing what is limited, I think it's okay to use max rather than max_computed or max_computations. The default values for both limit and max will be None.
eywalker commented on Oct 18, 2017
I feel a bit uneasy about having limit and max as I feel that that can be confusing as to what does exactly what.
dimitri-yatsenko commented on Oct 18, 2017
True, but they are easy to remember once you read the doc string. The trouble with longer names is that they are harder to remember even when you know what they do. I favor simply limit and max.
dimitri-yatsenko commented on Oct 19, 2017
max is a function is Python, so I would rather not use it. How about max_calls?
fixed datajoint#375 -- added the max_calls argument to populate
eywalker commented on Oct 19, 2017
max_calls sounds fine to me.
chbehrens commented on Jan 8, 2018
The usage of the newly added argument max_calls seems to throw a type error:
--> 126 if max_calls is not None and call_count >= max_calls: 127 break TypeError: unorderable types: itertools.count() >= int()