Skip to content
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

parallelize graph-building #22

Open
rkurchin opened this issue Oct 19, 2020 · 6 comments
Open

parallelize graph-building #22

rkurchin opened this issue Oct 19, 2020 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@rkurchin
Copy link
Member

It's a one-time cost for a given dataset, but it's quite slow (~1k/hr) for larger sets, and should be easy to batch out in some reasonably automated way...

@rkurchin rkurchin added the enhancement New feature or request label Oct 19, 2020
@rkurchin rkurchin self-assigned this Oct 19, 2020
@thazhemadam
Copy link
Member

thazhemadam commented Mar 14, 2021

@rkurchin Do you have something specific in mind, or would something like getting this loop to run on different threads work well enough?

@rkurchin
Copy link
Member Author

yeah something like that would be great! I didn't have anything more specific than that in mind, perhaps just adding an argument to specify number of threads (and/or perhaps a flag of whether to parallelize at all and it'll figure out how many there are) to the batch build function...

@thazhemadam
Copy link
Member

thazhemadam commented Mar 19, 2021

hmm. I'll try and see how we implement at least that much (for now).
If successful, I'll also try actually benchmarking the "before" and "after" (using something like BenchmarkTools.jl), and have those results also posted.

@thazhemadam thazhemadam self-assigned this Mar 19, 2021
@thazhemadam
Copy link
Member

thazhemadam commented Mar 21, 2021

Running that loop on different threads directly (using Threads.@threads or even Threads.@spawn ) causes a segmentation fault. This seems to be caused because of this line.
(Some more about PyCall and thread safety can be found here.

This could mean that we might have to try implementing some kind of synchronous locking mechanism that ensures only one thread can call pyimport_conda() and aseio.read() at a time, and see how that works out. But seeing how many pyimport_conda() calls get made for each build_graph() call, I'm not sure how much of a performance gain we'll get by parallelizing this. If anything, I feel this may just end up making the code unnecessarily complicated and unreadable.

@rkurchin
Copy link
Member Author

Have you tried using Distributed and just a pmap? I've had some success with that when parallelizing this stuff "manually" so maybe something about the way that handles stuff internally plays nicer with PyCall? I'm far from an expert on the internals there, but worth a shot.

@thazhemadam
Copy link
Member

Yes I tried that, and it still results in a segmentation fault. FWIW, I tried two variants of the function called in pmap - one called only pyimport_conda(), and the other called only aseio.read() (with aseio declared as a global variable).

@rkurchin rkurchin added this to To do, eventually in ChemistryFeaturization Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
ChemistryFeaturization
To do, eventually
Development

No branches or pull requests

2 participants