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 batch image generation and extended logging. #35

Closed
wants to merge 2 commits into from

Conversation

Godofnothing
Copy link
Contributor

Hi, thanks for your work. The results are quite impressive. Below I added a few modifications that can be helpful for experimentation.

The original src/generate.py generates images one-by-one which leads to under utilization of GPU and as consequence, generation of 30k images takes a while. I've added batched generation of images to speed-up it.

In addition, the base training script src/kd_train_text_to_image.py logs only the total loss to W&B and one may be interested in each particular contribution. I added image logging to W&B as well.

@bokyeong1015
Copy link
Member

Hi, thank you for your efforts in enhancing this repository!

TL;DR - We will get back to you after reviewing the functionality, generation score, GPU memory usage, and inference time. Appreciate your patience.


In addition, the base training script src/kd_train_text_to_image.py logs only the total loss to W&B and one may be interested in each particular contribution. I added image logging to W&B as well.

It looks great and seems working properly. We'll verify its functionality and reach out to you.

The original src/generate.py generates images one-by-one which leads to under utilization of GPU and as consequence, generation of 30k images takes a while. I've added batched generation of images to speed-up it.

We’d like to clarify that our src/generate.py with per-image inference was provided to reproduce the generation scores reported in our paper.
We need to examine how batch inference might influence these scores, particularly considering its potential effects on the sampling of random latent codes. Furthermore, the impact of batch size on GPU memory consumption and inference time will also be checked. This evaluation will take some time and thus might cause the delay in our response. We appreciate your kind understanding.

@Godofnothing
Copy link
Contributor Author

@bokyeong1015 Thanks for your response. Surely, reproducibility is crucial. I hope you'll find these modifications beneficial.

@bokyeong1015
Copy link
Member

Hi, thanks again for your PR and sorry for the late response.
We’ve carefully reviewed the modified code and would like to share our progress.

Before proceeding, would you kindly create separate PRs with two distinct branches? If this is OK for you, please let us know. We'll then set up the corresponding issues and branches and get back to you.

  • For instance, one branch could be named "improved-wandb-logger" and the other "batched-image-generation".
  • This would facilitate our discussion on the individual functionalities and allow us to make minor commits from our end.

Reviewing improved-wandb-logger (src/kd_train_text_to_image.py)

Thank you for adding this functionality; it works well.
One point we'd like to revise is logging {train_loss_sd, train_loss_kd_output, train_loss_kd_feat} instead of {loss_sd, loss_kd_output, loss_kd_feat}. This change would align with our custom CSV logging and enable considering gradient_accumulation_steps and distributed processes. If you create a separate PR, we'll reiterate this comment during the code review phase.


Reviewing batched-image-generation (src/generate.py, src/generate_single_prompt.py, src/utils/inference_pipeline.py)

We appreciate this valuable feature and would like to incorporate it with a default batch size of 1.
Though the inference batch size doesn't influence the reproducibility of generation scores (please see below), minimizing GPU memory usage would be beneficial for users with lower-spec machines. We'll provide a detailed explanation of this feature in our documentation and will appropriately acknowledge your contribution.

  • Results with the modified code (setup: BK-SDM-Small and MS-COCO 30K image generation)
    • measured on a single 3090 GPU (24GB GPU Memory)
    • the batch sizes of 12 and 16 were not runnable on 3090 (CUDA out of memory).
Eval Batch Size 1 4 8
GPU Memory 4978MiB (4.9GB) 11590MiB (11.3GB) 20034MiB (19.6GB)
Generation Time 34130s (9.5h) 27564s (7.7h) 26313s (7.3h)
FID 16.98 17.16 16.97
IS 31.68 31.62 31.22
CLIP Score 0.2677 0.2677 0.2675

@Godofnothing
Copy link
Contributor Author

Godofnothing commented Sep 27, 2023

@bokyeong1015 glad to hear you found these modifications helpful. Yeah I agree, that two pull requests for two separate features are better for maintaining the structure of the project, and I'll take into account your comments and points in the new pull requests.

This was referenced Sep 28, 2023
@bokyeong1015
Copy link
Member

Thanks for your kind reply! Could you please create two separate PRs for the branches 37-improved-wandb-logger and 38-batched-image-generation?

Either of the two options below is fine, so please decide at your convenience. Thank you for your time and help.

  • You can submit the PR with the current code as it is, and we can add minor commits on our end.
  • You can make the changes according to the above comments.

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.

2 participants