There are some problems with your code… but not the one you're asking asking about.
Since you didn't provide enough to run anything, I added this extra stuff:
class Genome(object):
i = 0
def __init__(self, newi = None):
if newi is None:
newi = Genome.i
Genome.i += 1
self.i = newi
def __repr__(self):
return 'Genome({})'.format(self.i)
def children(self):
return self._children
g1, g2 = Genome(), Genome()
g1._children = [Genome(), Genome()]
g2._children = [Genome(), Genome(), Genome()]
a_list_of_genomes = [g1, g2]
all_genomes = [g1.children()[0], g2.children()[2]]
Now, your algorithm should give us genomes #2 and #6. So, let's try your non-threaded code:
valid_children = set()
for focus_genome in a_list_of_genomes: # a list of objects
for child in focus_genome.children(): # a list of objects (again)
if child in all_genomes:
valid_children.update(set([child]))
print(valid_children)
I get {Genome(2), Genome(6)}, which is correct.
Now, your threaded code. Copying and pasting the inner loop body as the function body to make sure it's identical:
def threaded_function(focus_genome):
for child in focus_genome.children(): # a list of objects (again)
if child in all_genomes:
valid_children.update(set([child]))
Running it as you tried:
for focus_genome in a_list_of_genomes: # a list of objects
t = threading.Thread(target=threaded_function, args=(focus_genome,))
t.start()
t.join()
print(valid_children)
I get {Genome(2), Genome(6)}, which is not empty as you claim, and correct, and exactly the same as the non-threaded version.
That being said, you're not actually doing anything useful here—and, if you did, you'd have a problem.
First, join sits around waiting for the background thread to finish. So, there is no benefit in starting a thread and immediately joining it. Instead, you need to start a bunch of threads, then join all the threads. For example:
threads = [threading.Thread(target=threaded_function, args=(focus_genome,))
for focus_genome in a_list_of_genomes]
for thread in threads:
thread.start()
for thread in threads:
thread.join()
But if the threads are doing nothing but running CPU-intensive Python code, this won't help anyway, because the Global Interpreter Lock ensures that only one thread can run Python code at a time. Threads are great when you spend all your time doing I/O (reading files or HTTP URLs), waiting on user interaction, or calling slow functions in a handful of libraries like NumPy that are designed for threading. But for running Python code in parallel, they're not going to speed things up at all. For that, you need processes.
Meanwhile, you've got multiple threads trying to mutate a shared object, without any kind of synchronization. That's a race condition, which will lead to corrupted data. You need to use a lock or other sync object to protect shared data if you want to use threads:
valid_children_lock = Lock()
def threaded_function(focus_genome):
for child in focus_genome.children(): # a list of objects (again)
if child in all_genomes:
with valid_children_lock():
valid_children.update(set([child]))
And this kind of mutable-shared-data threading gets even worse when you're using processes. If you try to directly share a set between two processes, it sometimes works on Unix, and never on Windows.
If you can reorganize your logic to not use mutable shared data, everything gets a lot easier. One really easy way to do this is to write everything in terms of tasks that take parameters and return values—that is, functions without side-effects. Then you can just use a thread pool or executor to run all those tasks and give you back the results. Which has the added advantage that you run as many tasks at a time as you have workers, queuing the rest up automatically, instead of trying to run all of them at once (which is a lot faster).
Can we do that here? Maybe. If each task returns a set of valid_children found for a given focus_genome, then we can just union together all those sets and get the complete result, right? So:
def threaded_task(focus_genome):
valid_children = set()
for child in focus_genome.children(): # a list of objects (again)
if child in all_genomes:
valid_children.add(child)
return valid_children
valid_children = set()
with multiprocessing.Pool() as pool:
for subset in pool.imap_unordered(threaded_task, a_list_of_genomes):
valid_children.update(subset)
We could simplify this even further by using a couple of comprehensions:
def threaded_task(focus_genome):
return {child for child in focus_genome.children() if child in all_genomes}
with multiprocessing.Pool() as pool:
valid_children = set.union(pool.imap_unordered(threaded_task, a_list_of_genomes))
Nonevalues, just for side effects buried under the covers. Not to mention that it doesn't fit on an 80-column screen (or an SO question) without horizontal scrolling.valid_children.update(set([child]))is a very convoluted way of writingvalid_children.add(child). Why are you creating a 1-element list, then converting it to a 1-element set, all just to get that 1 element added to another set?thread = Thread(…); thread.start(); thread.join()in the loop instead of putting the code directly inside the loop. So, your problem appears to be in the code that you didn't show us (which I had to guess at), rather than the code that you did. Which means we can't debug it for you. Which is exactly why you should always provide an SSCCE instead of just random fragments of code.{Genome(2), Genome(6)}, proving that it does update the set.