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

move to predict_on_batch #78

Merged
merged 5 commits into from
Nov 16, 2022
Merged

move to predict_on_batch #78

merged 5 commits into from
Nov 16, 2022

Conversation

jeromelecoq
Copy link
Collaborator

The goal of this PR is to move to using predict_on_batch for inference since that should be both memory and computation efficient.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #78 (57ca75a) into master (8a7834c) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   51.49%   51.61%   +0.12%     
==========================================
  Files          11       11              
  Lines        1610     1614       +4     
==========================================
+ Hits          829      833       +4     
  Misses        781      781              
Impacted Files Coverage Δ
deepinterpolation/__init__.py 100.00% <100.00%> (ø)
deepinterpolation/cli/schemas.py 93.91% <100.00%> (+0.16%) ⬆️
deepinterpolation/inferrence_collection.py 65.07% <100.00%> (ø)
deepinterpolation/trainor_collection.py 75.00% <0.00%> (ø)

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 8a7834c...57ca75a. Read the comment docs.

@jeromelecoq
Copy link
Collaborator Author

@danielsf @aamster any concerns with this simple PR?

@aamster
Copy link
Collaborator

aamster commented Dec 6, 2021

@jeromelecoq I am not sure I understand why this fixes the issue. local_data[0] is a minibatch of data of type Sequence, correct? So predict will iterate through the dataset, but predict_on_batch will just predict on the entire minibatch without iterating. It seems like predict might be slower if the input is larger, but shouldn't use more memory.

The keras docs say to call the model directly for small inputs, ie self.model(local_data[0], training=False). Does that also fix the issue?

@jeromelecoq
Copy link
Collaborator Author

The issue is I think due to a memory leak. I found that looping with predict and GPUs will try to create huge arrays on the GPU. see here: #77

@jeromelecoq
Copy link
Collaborator Author

It also seemed like this was the expected use in our case, since I am actually feeding one batch of data as described here : https://stackoverflow.com/questions/44972565/what-is-the-difference-between-the-predict-and-predict-on-batch-methods-of-a-ker

@aamster
Copy link
Collaborator

aamster commented Dec 6, 2021

@jeromelecoq I am still confused as to why predict is running into a memory issue if local_data[0] is only a single minibatch, since, as the docs say, the default batch_size on predict is 32, and I believe local_data[0] has a batch size of 5, correct? In which case it should just take the entire input and do a forward pass on it, which is the same as predict_on_batch.

Are you able to try using self.model(local_data[0], training=False) which the docs suggest for small inputs? If not then it is fine.

@jeromelecoq
Copy link
Collaborator Author

jeromelecoq commented Dec 6, 2021

@jeromelecoq I am still confused as to why predict is running into a memory issue if local_data[0] is only a single minibatch, since, as the docs say, the default batch_size on predict is 32, and I believe local_data[0] has a batch size of 5, correct? In which case it should just take the entire input and do a forward pass on it, which is the same as predict_on_batch.

Yes batch size is usually set to 5 but in this case it is set by the generator so is user-defined. I am with you on not knowing why predict gives a GPU memory issue given the doc. But I can read from the doc that looping predict 1000 times might not be what they are testing for since predict is designed to break down batches internally. In addition, the array allocation request in the error is completely unexpected given the network architecture.

Are you able to try using self.model(local_data[0], training=False) which the docs suggest for small inputs? If not then it is fine.

I can try that, why do you think that would be superior to .predict_on_batch? Can you point me at the doc where this is described?
Thanks!

@aamster
Copy link
Collaborator

aamster commented Dec 6, 2021

Here it says

For small amount of inputs that fit in one batch, directly using call() is recommended for faster execution, e.g., model(x), or model(x, training=False) if you have layers such as tf.keras.layers.BatchNormalization that behaves differently during inference.

I am just worried that they don't mention predict_on_batch in their docs here, though it is probably fine.

@jeromelecoq
Copy link
Collaborator Author

Ah I see, I am also confused now why do they have predict_on_batch and call options?

@jeromelecoq jeromelecoq merged commit 2533346 into master Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants