Skip to content

Remove external logger dependency#2327

Merged
riccardofelluga merged 3 commits intomainfrom
change-peft-bmk-logger
Jul 21, 2025
Merged

Remove external logger dependency#2327
riccardofelluga merged 3 commits intomainfrom
change-peft-bmk-logger

Conversation

@riccardofelluga
Copy link
Collaborator

@riccardofelluga riccardofelluga commented Jul 14, 2025

What does this PR do?

After discussing with @crcrpar we thought it's better to remove the dependency to loguru and not use #2275.

The log looks like:

25-07-14 15:42 INFO Global batch size: 1 (mbs: 1, grad_acc_steps: 1, world_size: 1)
25-07-14 15:42 INFO Loading tokenizer
25-07-14 15:42 INFO Loading base model on meta device...
25-07-14 15:42 INFO Base model loaded on meta device
25-07-14 15:42 INFO Configured model for static shapes with sequence length: 4096
25-07-14 15:42 INFO Gradient checkpointing disabled
25-07-14 15:42 INFO Applying LoRA to model
25-07-14 15:42 INFO LoRA applied to model
25-07-14 15:42 INFO Verifying gradient setup...
25-07-14 15:42 INFO Applying compilation: thunder to model
25-07-14 15:42 INFO Resetting cache size for torch.dynamo
25-07-14 15:42 INFO Disabled gradient checkpointing for Thunder compilation
25-07-14 15:42 INFO Thunder used executors: ['cudnn', 'sdpa', 'torchcompile_xentropy', 'nvfuser']
25-07-14 15:42 INFO Applying Thunder compilation with 4 executors
25-07-14 15:42 INFO Using ThunderFX
25-07-14 15:42 INFO Compilation applied to model
25-07-14 15:42 INFO Trainable parameters: 2,883,584
25-07-14 15:42 INFO Total parameters: 1,558,997,504

cc @Borda

Copy link
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

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

I am fine with trimming dependencies, but could you pls share the motivation?

@crcrpar
Copy link
Collaborator

crcrpar commented Jul 16, 2025

I personally would find it better to remove a benchmark specific dependency if it's not that difficult to remove it and the required code change isn't overwhelming

@riccardofelluga
Copy link
Collaborator Author

@Borda

I am fine with trimming dependencies, but could you pls share the motivation?

Yess so this came up in the PR that merged the benchmark script #2254 (review) and I've told @t-vi that I would try to move from this dep to the internal Thunder logger.

However the logger is better reserved to Thunder internals and therefore I've just removed the loguru dependency.

Copy link
Collaborator

@t-vi t-vi left a comment

Choose a reason for hiding this comment

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

Thank you @riccardofelluga

@riccardofelluga riccardofelluga merged commit b5d3c3a into main Jul 21, 2025
53 checks passed
@riccardofelluga riccardofelluga deleted the change-peft-bmk-logger branch July 21, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants