Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Switch output writing to h5py to reduce memory footprint. Update version #12

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

thomas-a-neil
Copy link

Collecting all data and then pickling can crash a machine with limited memory.

e.g. a 20GB results set (held in memory) will crash a 32GB machine. This is partially due to the memory overhead of pickling an object. Writing to hdf5 allows us to write iteratively, reducing the memory footprint overall, and avoiding the pickling overhead.

@DavidMChan
Copy link
Member

DavidMChan commented Aug 9, 2019

Can we change this so that the behavior remains the same for code that already exists - and add a new option, "save_format" with a default to "pkl"? This would break some code that already exists - as it relies on the outputs in a pkl format. Also, I've changed this branch to develop - but that might require some different commits - it looks like some things are changed that should not be, when merging.

@DavidMChan DavidMChan closed this Aug 9, 2019
@DavidMChan DavidMChan reopened this Aug 9, 2019
@DavidMChan
Copy link
Member

Whoops, on my phone - closed this by accident.

@thomas-a-neil
Copy link
Author

That makes sense. I'll add the flag and rebase to develop

@codecov-io
Copy link

codecov-io commented Aug 9, 2019

Codecov Report

Merging #12 into develop will decrease coverage by 0.14%.
The diff coverage is 5.55%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #12      +/-   ##
===========================================
- Coverage     65.6%   65.46%   -0.15%     
===========================================
  Files          143      143              
  Lines         6240     6255      +15     
===========================================
+ Hits          4094     4095       +1     
- Misses        2146     2160      +14
Impacted Files Coverage Δ
rinokeras/core/v1x/train/RinokerasGraph.py 27.7% <5.55%> (-2.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f61c48a...0927555. Read the comment docs.

while True:
loss, outputs = self.run('default', return_outputs=True)
grp = f.create_group(str(i))
outputs = outputs[0] # can we rely on this being a tuple of length 1?
Copy link
Author

Choose a reason for hiding this comment

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

@DavidMChan I'm not sure about this, but can I rely on the run output always being a tuple of length 1?

Copy link
Member

Choose a reason for hiding this comment

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

Effectively, the outputs are the forward pass of your model. This means that "outputs" can be whatever you want it to be (a numpy array, a dict of numpy arrays, a tuple of arrays, etc.) This is probably why @rmrao hijacked it for use in TAPE. It also makes it tricky to write a generic saving function for the outputs since you have no guarantees on the data format. You can know, however, that the outputs will be the result of a forward run of the model (so they are convertible to tensors).

Perhaps it makes sense instead of adding options, to add a callback function? Not entirely sure, but this is why we used pickle.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of a callback function. The save_outputs option is really more about quick-and-dirty debugging than it is a real feature at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise there's not a really good, general way of saving things. It'll vary hugely. Plus a callback would let us do things other than saving them.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants