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

added thread lock acquire and release to OnDemandLinkSequence, to allow multithreading #477

Merged
merged 2 commits into from Aug 15, 2019

Conversation

@youph
Copy link
Contributor

youph commented Aug 15, 2019

This patch, inspired by this post, appears to have fixed the multithreading issue. The way to use is

model.fit_generator(
    train_gen,
    epochs=epochs,
    verbose=1,
    use_multiprocessing=False,
    workers=4,
    shuffle=True,
)

@PantelisElinas the speed up is significant on CPU; let's try this on fractal using GPU.

…ow multithreading
@youph youph requested review from PantelisElinas and geoffj-d61 Aug 15, 2019
@youph youph added the ml label Aug 15, 2019
@codeclimate

This comment has been minimized.

Copy link

codeclimate bot commented Aug 15, 2019

Code Climate has analyzed commit b42d4c4 and detected 0 issues on this pull request.

View more on Code Climate.

@youph youph self-assigned this Aug 15, 2019
Copy link
Contributor

geoffj-d61 left a comment

Simple fix, looks good. Do you have the CPU timing results, with and without the fix?

@youph

This comment has been minimized.

Copy link
Contributor Author

youph commented Aug 15, 2019

Simple fix, looks good. Do you have the CPU timing results, with and without the fix?

Yes, the training time per epoch on 2.9 GHz Intel Core i7 CPU is:
with workers=0 (executing the generator on the main thread): 83s/epoch
with workers=1: 63s/epoch
with workers=2: 37s/epoch
with workers=4: 32s/epoch

Before the fix, an error was thrown ValueError: generator is already executing; this PR fixes that.

@PantelisElinas

This comment has been minimized.

Copy link
Contributor

PantelisElinas commented Aug 15, 2019

Hi @youph

the fix looks good and I have not come across the exception we used to get. Also, here are the timing results from our workstation.

Times are per epoch on CPU and GPU using multi-threading. The machine is Intel i9-7960X 2 2.80GHz with NVIDIA GeForce RTX 2080.

CPU:
workers=0: 37s
workers=2: 18s
workers=4: 14s
workers=6: 14s
workers=8: 14s

GPU:
workers=0: 42s
workers=2: 17s
workers=4: 19s
workers=8: 18s

GPU utilisation for the above experiments was 10-15% (very low) but we already know that with GraphSAGE we have a bandwidth problem sending the node features to the GPU. In other words, there is little benefit using the GPU, in fact, the extra overhead of having to send data to the GPU hurts performance.

Regards,

P.

@youph youph merged commit 57a0a1f into develop Aug 15, 2019
3 checks passed
3 checks passed
buildkite/stellargraph Build #1491 passed (7 minutes, 57 seconds)
Details
coverage/coveralls First build on fix/OnDemandLinkSequence at 85.886%
Details
license/cla All CLA requirements met.
@youph youph deleted the fix/OnDemandLinkSequence branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.