parallel implementation for all_pairs_bellman_ford_path

[Schefflera-Arboricola]

networkx/networkx#7003

Also, for a larger number of nodes, I was getting this heatmap :

Please give your feedback.

Thank you :)

[dschult]

The helper function could be simplified (and maybe sped up) by using a dict comprehension:

def _calculate_shortest_paths_subset(G, chunk, weight):
    return {n: single_source_bellman_ford_path(n, weight=weight) for n in chunk}

and it might also be easy (since it is so short) to define the helper function inside the function itself. It's a style choice. If there is a chance that it could be used by another function, then keep it defined in the main module.

I have a feeling that some of these idioms will be used over and over. Like:

    num_in_chunk = max(len(nodes) // total_cores, 1)
    node_chunks = nxp.chunks(nodes, num_in_chunk)

PR #7 has consolidated some of those into utility functions that most of the functions in nx_parallel could use. There already is one utility function you have used: nxp.cpu_count(). Maybe there should be others. I'm not sure what the best way to implement things like that is. But we should think about it. No need to implement those ideas for this PR though.

:)

[Schefflera-Arboricola]

@dschult There wasn't much difference in the speedups from the above dict comprehension also. The parallel implementation is taking more time because here I am computing all the paths and returning them in a dictionary but in the non-parallel implementation an iterator is returned so the paths are yielded(computed) when/if the iterator is iterated later in the program. And we cannot break down the task, such that, we compute multiple generators in parallel and then combine them, like this :

def _calculate_shortest_paths_subset(G, chunk, weight):
    for n in chunk:
        yield n, single_source_bellman_ford_path(G, n, weight=weight)

because joblib requires a serializable data structure to be returned to apply parallization(error: TypeError: cannot pickle 'generator' object).

But if someone wants to compute all the paths then this implementation is better than iterating through the iterator of the non-parallel function. (Suggestion(feature): add a parameter iter in the all_pairs_bellman_ford_path function, which when is True would return an iterator object and a dictionary when it's False) Here, the time to convert the iterator into a dictionary is also taken into account (timing file)

            t1 = time.time()
            c = currFun(H)
+           d1=dict(c)
            t2 = time.time()
            parallelTime = t2 - t1
            t1 = time.time()
            c = currFun(G)
+           d2=dict(c)
            t2 = time.time()
            stdTime = t2 - t1
            timesFaster = stdTime / parallelTime
            heatmapDF.at[j, i] = timesFaster
            print("Finished " + str(currFun))

here, we are getting speedups :

Also, thanks for the feedback on styling the code!
Please let me know what you think and how I should proceed from here.

Thank you :)

[dschult]

I think you can handle generators with joblib.
Take a look at this page and maybe a page linked from this one.
https://joblib.readthedocs.io/en/stable/parallel.html
Hopefully it is simple to do.

my understanding is that each cpu generates values, but they are yielded to the user in the order they are started, so if one is faster than a previous one, it waits until the previous one is finished and yielded before being yielded itself. The order is preserved relative to the non-parallel version.

now, the timing should still include generating the entire set of nodes. Because setting up the generators take very little time. It is when they actually do the computations that it takes time.

[Schefflera-Arboricola]

thanks @dschult

I have updated it now. Also, I have used weighted graphs this time. And, the speedup values seem to decrease after the 100-node graph. Let me know if there's anything else to improve upon.

Thank you :)

[dschult]

This looks good -- I have some comments below, but I think this is close to being ready.
:)

[dschult]

I think G and weight are already loaded into the outer function's namespace, so they will be found when used within this helper function. So you can remove those two inputs and make this a function of only source. That also shortens the later code that calls this function. Less time patching together function arguments, more time needed for variable lookups. But I think it could be faster overall. Can you tell?

[Schefflera-Arboricola]

yes, passing only source is a little faster, following are the speed-ups for the same :

I have changed it in the recent commit

[dschult]

You should probably set the seed for random functions when timing. That will help ensure that the same steps are taken by the various trials. This is true for random.random() and also the graph creation routines.

[dschult]

Is it worth checking that the results are the same? (outside the timing part)
something like assert d1 == d2.

[Schefflera-Arboricola]

I checked d1==d2 for the all_pairs_bellman_ford_path before committing. It was true for all cases. But, for betweenness_centrality it was not always true. I had to round up all the values. I can add separate tests for all the algorithms, if that seems good to you.

[Schefflera-Arboricola]

@dschult I have made all the updates please let me know if this looks good to you. Thank you very much for the review :)

[Schefflera-Arboricola]

I tried running the same timing_individual_function.py script again and I got the following heatmap :

This is the heatmap right now in the PR :

Would you say it's a lot of variation, especially for the 100-node graphs?

Thank you :)

[dschult]

I think this looks good.
There are some big-picture issues we should probably consider at some point -- but not for this PR's intent (addition of a parallel implementation).

Some of the questions invovle things like:

• should we include the NX docs information in the nx-parallel doc_strings? That might lead to one version becoming out-of-date compared to the other. Perhaps we should only put info about the parallel implementation here and refer to the NX docs for the info about the original function. If so, where do we draw the line? at the function signature? do we copy the parameter descriptions?
• Should we centralize the 'chunk'ing and 'map'ing and 'reduce'ing? see [WIP]: Refactor-- consolidate and simplify #7

But let's go ahead and merge this and worry about the big picture issues in another PR.
Thanks!

[MridulS]

We should be moving this to the function signature and match something like what scikit-learn does (n_jobs) but not a blocker here.

[MridulS]

Thanks for this @Schefflera-Arboricola !

There are a bunch of stuff we should revisit, especially in the context of #7 but for now let's merge this in :)