-
Notifications
You must be signed in to change notification settings - Fork 102
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
Precomputation folder name based on model name #196
Conversation
finetrainers/trainer.py
Outdated
@@ -30,6 +29,8 @@ | |||
from peft import LoraConfig, get_peft_model_state_dict, set_peft_model_state_dict | |||
from tqdm import tqdm | |||
|
|||
import wandb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to ignore this from ruff? If I have a wandb folder in the root directory from some training run, and then run make style
, it just moves it down here for some reason
finetrainers/trainer.py
Outdated
@@ -252,8 +252,14 @@ def collate_fn(batch): | |||
"Caption dropout is not supported with precomputation yet. This will be supported in the future." | |||
) | |||
|
|||
conditions_dir = Path(self.args.data_root) / PRECOMPUTED_DIR_NAME / PRECOMPUTED_CONDITIONS_DIR_NAME | |||
latents_dir = Path(self.args.data_root) / PRECOMPUTED_DIR_NAME / PRECOMPUTED_LATENTS_DIR_NAME | |||
conditions_dir = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nits):
- If we use the same model type but use a different checkpoint, it might still lead to some inconsistencies (in case we have different dtypes for the VAE, etc.). So, would prefer to also append the checkpoint here besides
model_name
. - Would assign
Path(self.args.data_root) / f"{self.args.model_name}_{PRECOMPUTED_DIR_NAME}"
to a variable and reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh okay yeah makes sense, will update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maybe look into custom component based naming in the future because atm only loading ALL components from a single model_id is supported.
With the latest commit, the folder created looks something like: /raid/aryan/video-dataset-disney/ltx_video_a-r-r-o-w-LTX-Video-diffusers_precomputed/
. Is this what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me thanks!
Currently, if we try to train two models with same datasets and enabling precomputation, it is not possible. This PR adds model_name to the folder so that the name clash does not happen
Tested on smol run for LTX:
Script