why does populate need to return nkeys now?

This can be useful in automating workflows, since it allows us to determine how many keys were found to be processed.
For example, this features is used in the neurophotonics project in check_db. This allows the EC2 instance to continue working until the user stops inserting keys into the database, for those times that keys are inserted after the instance started, but before the initial workload is finished.

Current users expect populate to return the error list. This change may break existing code. Changing the API should be done carefully.

That is a valid concern, but I think it is also useful to be able to tell how many keys were processed.

I notice that there is nothing returned if suppress_errors == False, the default value. What if the return was left as just error_list in the case suppress_errors == True, and returned nkeys only if suppress_erros == False?

here error_list is empty. Why does this need to be return? What's the use case for processes == 0?

In this case, error_list will indeed always be an empty list, but I think it is best to return it anyway. If we do it like this, then autopopulate will always return a list and an integer, regardless of which case this happens to be. Since we can always expect the same return type, we will never have to later worry about which case we will encounter.

As for the use case of processes == 0, this is the case which caused the error the PR is meant to solve. Consider the case in which you are using multiprocessing (that is, when you call this function, you have set processes to be greater than one), but the table you are populating has no keys remaining to process. This case occurred when automating the workflow of the neurophotonics project, since populate was being called on every table.

In this case, we would like for the function to do nothing and move on to the next task. Instead, it throws a value error. This is because of lines (215-216) in the autopopulate function.

if processes > 1:
            processes = min(processes, nkeys, mp.cpu_count())

Since processes has been set to a value greater than 1, the second line here runs. Since the table has already been populated in this case, nkeys = 0. Since there is no reason for any of these three values to be negative, the minimum is 0. Thus processes=0 at this point.

Previously, if processes was not equal to 1, we would try to spawn multiple processes equal to processes. Since 0 != 1, we try to call multiprocessing with a pool of 0 processes, which created a value error.

This line prevents this exception from being thrown by handling the case in which processes == 0. Now, the function simply reports that there were no errors and no keys to process, and moves on.

I think if nkeys <= 0, it should just quietly exit. That's consistent with previous behavior.

In the case nkeys == 0, it does quietly exit. It returns ([], 0), but it doesn't mutate any values, initialize new variables or print anything to the screen. Users don't have to use the return value, but they can if they want to.

The case nkeys < 0 should never occur. My understanding is that this value represents the number of keys which need to be processed. If this value is negative, an error has occurred.

This is a great idea to improve how this can be specified but doesn't seem to be true based on what is here.

If you do in fact set process=None the following would happen:

• [216]: Should throw an error like: TypeError: '>' not supported between instances of 'NoneType' and 'int'

You could probably just replace that particular if-block with something like this:

min(nkeys, *([processes, mp.cpu_count()] if processes else [mp.cpu_count()]))

OR

min(*(_ for _ in (processes, nkeys, mp.cpu_count()) if _))

These approaches would leverage argument expansion to our benefit to expand only the relevant values into the min function.

oops good point.

With this, processes=0 will have the same effect as processes=None.