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.

Sign up for GitHub

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

Jump to bottom

Faster sequence #8039

Merged
merged 3 commits into from
Oct 2, 2017
Merged

Faster sequence #8039

merged 3 commits into from
Oct 2, 2017

Conversation

Dref360
Copy link
Contributor

@Dref360 Dref360 commented Oct 2, 2017

During the initial implementation of Sequence, there have been some issues when using large objects inside of the Sequence like a list of paths. This PR solves these issues by caching the Sequence in each process instead of copying it everytime we need it. Between epochs, we update the Sequence since it may get modified by the on_epoch_end.

I could have done it in a simpler way by direction accessing self.pool._processes, but it would be accessing a private member.

Example :
Let's say that we keep a list x inside a Sequence where x = [('abc', 512)] * 100000.

Old way : 1.8216075897216797 seconds
New way : 0.22916626930236816 seconds

fchollet
fchollet reviewed Oct 2, 2017
View reviewed changes
keras/utils/data_utils.py Outdated
# Global variables to be shared across processes
shared_sequence = None
manager = multiprocessing.Manager()
shared_dict = manager.dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use all caps for global variables, and use a _ prefix to indicate they are private.

keras/utils/data_utils.py Outdated
@@ -516,6 +530,18 @@ def get(self):
self.stop()
raise StopIteration(e)

def send_seq(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is only used by the Sequence instance. Please make it private.

keras/utils/data_utils.py Outdated
return shared_sequence[i]


def update_sequence(seq):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this method private and add a docstring explaining its purpose.

fchollet
fchollet approved these changes Oct 2, 2017
View reviewed changes
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@fchollet fchollet merged commit 19e1be2 into keras-team:master Oct 2, 2017
ozabluda pushed a commit to ozabluda/keras that referenced this pull request Oct 2, 2017
@ozabluda
Faster sequence (keras-team#8039)
4fcc167 
* Make Sequence faster

* Don't update if we're done

* Change according to review
@farizrahman4u
Copy link
Contributor

farizrahman4u commented Oct 3, 2017 •
edited
Loading

This breaks on windows.
Can't import Keras.

Note that there is no os.fork on Windows.

@Dref360
Copy link
Contributor Author

Dref360 commented Oct 3, 2017

Could you show the stacktrace? I don't have a Windows machine.
This is pretty much the same logic as before where it only used a Pool.
I guess that on Windows, using a multiprocessing.Manager is not good?
Do you know if Sequence were working before?

@Dref360
Copy link
Contributor Author

Dref360 commented Oct 3, 2017

I did a fix, the manager is only initialized if we need it. (Which saves memory for everyone if we do not use Sequence)
https://github.com/Dref360/keras/tree/fasterSequence

I do not have access to a Windows machine, is there a quick way to see if it fixes the bug?

@konstantinberlin
Copy link

What happens if you use a sequence in the generator, while the previous sequence is not closed?
For example, in a callback, on every epoch. Seems like only one sequence can be active at a time, since it is a single global variable.

@farizrahman4u
Copy link
Contributor

farizrahman4u commented Oct 3, 2017 •
edited
Loading

@Dref360 Yes, it works. Please feel free to ping me for all multiproc related PRs for testing on Windows.

@Dref360
Copy link
Contributor Author

Dref360 commented Oct 3, 2017

Will do. Great point Constantin! This is not handled now will be done tonight.

@konstantinberlin
Copy link

konstantinberlin commented Oct 4, 2017 •
edited
Loading

My concern maybe also the fact that the entire sequence is being send to every processes on end of epoch. Potentially this can easily blow the memory of the machine if sequence has a lot of information and you a lot of processors. For example, like someone mentioned, if you are linking to a file path for 100M files, this info will be copied to every single processes.

I think the better solution would be to simply restart the pool on every epoch, thus utilizing the copy on write mechanism, to save memory (at least on Linux, not sure about windows). The overhead of a pool start is not large, as compared to large data cases when you are copying GB of data to each processes.

@bdwyer2
Copy link
Contributor

bdwyer2 commented Oct 4, 2017

Fyi: #8058

Dref360 pushed a commit to Dref360/keras that referenced this pull request Oct 5, 2017
Revert "Faster sequence (keras-team#8039)"
b3cbecd 
This reverts commit 19e1be2.
@Dref360 Dref360 mentioned this pull request Oct 5, 2017
Merged
bdwyer2 added a commit to bdwyer2/keras that referenced this pull request Oct 5, 2017
@bdwyer2
Revert "Faster sequence (keras-team#8039)"
3b96ea0 
This reverts commit 19e1be2.
dschwertfeger added a commit to dschwertfeger/keras that referenced this pull request Oct 8, 2017
@dschwertfeger
Merge branch 'master' into tensorboard-callback-embeddings-from-layer…
98c76a9 
…-outputs

* master: (68 commits)
  Change default value of shuffle parameter of Sequential.fit_generator() from True to False. (keras-team#8075)
  Fix off-by-one bug in predict/evaluate progress bar (keras-team#8071)
  Revert "Faster sequence" (keras-team#8060)
  Support NCHW for conv2d. (keras-team#8021)
  Change compute_accuracy() argument order and names (keras-team#8049)
  Replace literal constant 10 with variable num_classes in example/ (keras-team#8041)
  Faster sequence (keras-team#8039)
  Improve RNN docs.
  Enable accuracy reporting during training in examples/mnist_siamese_graph.py (keras-team#7997)
  Bug fix: Models with shared layers shouldn't be considered Sequential like (keras-team#8025)
  Add 'subtract' merge layer documentation (keras-team#8038)
  Update inference in seq2seq script to be more efficient
  Remove lstm_benchmark from examples/README.md (keras-team#8024)
  Add shuffle to the Model API (keras-team#8023)
  Add seq2seq example script.
  fix travis failure (keras-team#8014)
  Improve TF backend's Switch function (keras-team#7958)
  Added support for dynamic noise_shape in Dropout (keras-team#7999)
  Make on_epoch_end optional (keras-team#8007)
  Incremental tests speed ups.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fchollet fchollet fchollet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Dref360 @farizrahman4u @konstantinberlin @bdwyer2 @fchollet