Skip to content

[QEff Finetune]: Use logger in place of print statements in finetuning scripts #371

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

quic-mamta
Copy link
Contributor

Use logger in place of print statements in finetuning scripts

@quic-mamta quic-mamta requested review from quic-swatia and vbaddi and removed request for ochougul and quic-rishinr April 21, 2025 09:08
@quic-mamta quic-mamta self-assigned this Apr 21, 2025
Signed-off-by: Mamta Singh <quic_mamtsing@quicinc.com>
Copy link
Contributor

@vbaddi vbaddi left a comment

Choose a reason for hiding this comment

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

we can merge this?

Signed-off-by: Mamta Singh <168400541+quic-mamta@users.noreply.github.com>
Copy link
Contributor

@quic-meetkuma quic-meetkuma left a comment

Choose a reason for hiding this comment

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

Overall, it is a good change, Mamta! Let us try to include more things as suggested.

@@ -63,6 +65,6 @@ def get_data_collator(dataset_processer, dataset_config):
try:
return getattr(module, func_name)(dataset_processer)
except AttributeError:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for all the cases like L62.

Whenever we are raising some exceptions, it is good to log the raised exception, otherwise dumped file logs would represent different picture than the console logs.

We can override the logger to have a custom log function as below.

 import logging

 class CustomLogger(logging.Logger):
     def raise_runtimeerror(self, message):
         self.error(message)
         raise RuntimeError(message)

 logging.setLoggerClass(CustomLogger)
 logger = logging.getLogger(__name__)
 logging.basicConfig(level=logging.DEBUG, filename='app.log', filemode='a',
                     format='%(asctime)s - %(name)s - %(levelname)s - %(message)s')

What do you say?

Copy link
Contributor

@quic-meetkuma quic-meetkuma left a comment

Choose a reason for hiding this comment

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

Address this one as well.

@@ -113,26 +114,26 @@ def train(
for epoch in range(train_config.num_epochs):
if loss_0_counter.item() == train_config.convergence_counter:
if train_config.enable_ddp:
print(
logger.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

One important change is required.
We need to log on console only for rank == 0 in case of ddp. Otherwise in 64x ddp or 48x ddp will fill the console with lot of information which is not user friendly.

quic-mamta and others added 2 commits May 16, 2025 12:27
Signed-off-by: Mamta Singh <mamtsing@blr-ubuntu-g293-22.qualcomm.com>
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