Fix: Removal of transformers logger and addition of python native logger#270
Conversation
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
…thon logger Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
…del-stack#258) * initial code for deleting lm_head Signed-off-by: Anh-Uong <anh.uong@ibm.com> * fix logic for copying checkpoint Signed-off-by: Anh-Uong <anh.uong@ibm.com> * fix check that embed_tokens and lm_head weights are the same Signed-off-by: Anh-Uong <anh.uong@ibm.com> * fix warning assertion Signed-off-by: Anh-Uong <anh.uong@ibm.com> * fix lm_head check, remove test Signed-off-by: Anh-Uong <anh.uong@ibm.com> * small fixes from code review Signed-off-by: Anh-Uong <anh.uong@ibm.com> * fmt Signed-off-by: Anh-Uong <anh.uong@ibm.com> --------- Signed-off-by: Anh-Uong <anh.uong@ibm.com> Co-authored-by: Anh-Uong <anh.uong@ibm.com> Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
68a91c5 to
773f0d1
Compare
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
anhuong
left a comment
There was a problem hiding this comment.
Thanks Abhishek for the logging update, I think this will help a lot. A few questions on how the logging works and the desired logic
build/accelerate_launch.py
Outdated
| ) | ||
|
|
||
| # Configure log_level of python native logger. | ||
| LOGLEVEL = None |
There was a problem hiding this comment.
nit: I had used an uppercase variable for this since it was pulling from the environment vars. But uppercase variables are usually used for constants, and since this isn't one, we can change it to a lowercase log_level
There was a problem hiding this comment.
Thank you for the PR review. This is done!
build/accelerate_launch.py
Outdated
| if "log_level" in job_config and job_config["log_level"]: | ||
| LOGLEVEL = job_config["log_level"].upper() | ||
| logging.basicConfig(level=LOGLEVEL) | ||
| else: | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) |
There was a problem hiding this comment.
small refactor you can do:
log_level = job_config.get("log_level") # this will be set to either the value found or None
if not log_level: # if log level not set by job_config aka by JSON, set it via env var or set default
log_level = os.environ.get("LOG_LEVEL", "WARNING")
log_level = log_level.upper()
logging.basicConfig(level=log_level)
os.environ["LOG_LEVEL"] = log_level # nice to move this up so all the parts related to log level are togetherThere was a problem hiding this comment.
This is done! Thank you for the input!
tuning/config/configs.py
Outdated
| default="passive", | ||
| metadata={ | ||
| "help": "The log level to adopt during training. \ | ||
| Possible values are 'debug', 'info', 'warning', 'error' and 'critical'" |
There was a problem hiding this comment.
Add note on what "passive" value means
plus a ‘passive’ level which doesn’t set anything and keeps the current log level for the Transformers library (which will be "warning" by default)
There was a problem hiding this comment.
Added note in metadata!
tuning/sft_trainer.py
Outdated
| # Assign value of either env var LOG_LEVEL or warning | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) | ||
| train_logger = logging.getLogger() |
There was a problem hiding this comment.
Since you call this on line 497, you don't need to call it within this block as well
There was a problem hiding this comment.
This is done!
tuning/sft_trainer.py
Outdated
| logger.debug( | ||
|
|
||
| # Function to set log level for python native logger and transformers training logger | ||
| training_args, _ = set_log_level(training_args) |
There was a problem hiding this comment.
Why are you not using the logging.getLogger() that you returned? Since you are using the root logger, running
logging.warning("Warning")
# above is the same as below
root_logger = logging.getLogger()
root_logger.warning("Warning")Running the above is the same. So if you are not using the getLogger, you shouldn't return it/use it.
But it looks like it is best practice to use getLogger from python docs and python logging howto.
So then instead of running logging.warning() you'd run logger.warning(). In which in the train() you would also call logging.getLogger(). It looks like its best practice to create module level loggers. So you could do logging.getLogger("sft_trainer_main") and logging.getLogger("sft_trainer_train")` and then the log messages should show like:
# named logger if you do getLogger("sft_trainer_train")
WARNING:sft_trainer_train:My warning
# root logger if you do getLogger()
WARNING:root:My warningThere was a problem hiding this comment.
In returned logger on line number 497, just to get logger in unit test cases functions. returning logger won't make sense in same file as we can set log level in same py file with returning logger.
But ya that's a good idea to use logging.getLogger. I have made the changes.
| self.logger = logging.get_logger("file_logging_tracker") | ||
| # Configure log level | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) |
There was a problem hiding this comment.
logging.basicConfig() sets the log level for the root logger. But you can set individual loggers level by running self.logger.setLevel(level)
There was a problem hiding this comment.
There are a few of these to fix, not going to comment in each file
There was a problem hiding this comment.
I don't think you even have to set log level for every file, you only have to set it once in the sft_trainer.py after parsing user's input right?
In each file, you just need to get a logger by doing:
import logging
logger = logging.getLogger(__name__)
and every time we log something, we just use that logger:
logger.info("Log this info")
There was a problem hiding this comment.
I agree that having each file set their own logging level helps to ensure that the logging level is set if a file is called outside of sft_trainer.py. I do agree that using logging.getLogger(__name__) is best practice and it is unlikely these files will be called outside of sft_trainer.py so I think going with Angel's suggestion works
tuning/sft_trainer.py
Outdated
|
|
||
| # Check if env var TRANSFORMERS_VERBOSITY is not set. | ||
| # Else if env var is already set then, log level of transformers is automatically set. | ||
| if os.environ.get("TRANSFORMERS_VERBOSITY") is None: |
There was a problem hiding this comment.
nit: can do
| if os.environ.get("TRANSFORMERS_VERBOSITY") is None: | |
| if not os.environ.get("TRANSFORMERS_VERBOSITY"): |
There was a problem hiding this comment.
Although do we even need the code block below? What use case is it covering? Do we have to check that TRANFORMERS_VERBOSITY at all?
is this trying to do
if os.getenv("TRANFORMERS_VERBOSITY"):
parsed_training_args.log_level = os.getenv("TRANFORMERS_VERBOSITY").lower()There was a problem hiding this comment.
I don't think we are required to check "TRANFORMERS_VERBOSITY". Hence I haven't added this code in final commit.
tuning/sft_trainer.py
Outdated
| if parsed_training_args.log_level != "passive": | ||
| logging.basicConfig(level=parsed_training_args.log_level.upper()) | ||
| else: | ||
| # Assign value of either env var LOG_LEVEL or warning | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) | ||
| train_logger = logging.getLogger() |
There was a problem hiding this comment.
Could this work to attain the logic we're looking for?
Goal for logic:
- TRANSFORMERS_VERBOSITY ---> should only set transformers logging, this is different than setting --log_level
--log_level--> sets both python and transformers logging- For consistency with the flag, LOG_LEVEL also sets python and transformers logging
if parsed_training_args.log_level != "passive":
logging.basicConfig(level=parsed_training_args.log_level.upper())
elif os.environ.get("LOG_LEVEL"):
# Assign value of either env var LOG_LEVEL or warning
LOGLEVEL = os.environ.get("LOG_LEVEL")
logging.basicConfig(level=LOGLEVEL)
# Set log_level in TrainingArguments
parsed_training_args.log_level = LOGLEVEL.lower()
train_logger = logging.getLogger()There was a problem hiding this comment.
I didn't add this line parsed_training_args.log_level = LOGLEVEL.lower() in the same if-else because if env variable 'TRANSFORMERS_VERBOSITY' is set, then this would get overwritten. Hence wrote this line in separate log below checking if not os.environ.get("TRANSFORMERS_VERBOSITY"):
Also I think in your code above in else logic we can add logging.basicConfig(level="WARNING") as default.
Thanks for the optimized code idea. Taking above points in consideration too I have optimized the code to work according to our logic. Feel free to comment on it.
log_level = None
if parsed_training_args.log_level != "passive":
log_level = parsed_training_args.log_level
elif os.environ.get("LOG_LEVEL"):
log_level = os.environ.get("LOG_LEVEL")
parsed_training_args.log_level = (
log_level.lower()
if not os.environ.get("TRANSFORMERS_VERBOSITY")
else os.environ.get("TRANSFORMERS_VERBOSITY")
)
else:
log_level = "WARNING"
logging.basicConfig(level=log_level.upper())
train_logger = logging.getLogger()
There was a problem hiding this comment.
Let's chat about this logic to clarify -- update: done!
tuning/sft_trainer.py
Outdated
| logging.basicConfig(level=parsed_training_args.log_level.upper()) | ||
| else: | ||
| # Assign value of either env var LOG_LEVEL or warning | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() |
There was a problem hiding this comment.
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | |
| LOGLEVEL = os.environ.get("LOG_LEVEL", logging.WARNING).upper() |
There was a problem hiding this comment.
Here if LOG_LEVEL is not set, it will default to logging.WARNING, which is an integer constant from the logging module (default is 30). The use of .upper() here will cause an error because .upper() is a string method and cannot be called on an integer.
| self.logger = logging.get_logger("file_logging_tracker") | ||
| # Configure log level | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) |
There was a problem hiding this comment.
It'd be helpful also if in each file, you could do:
logger = logging.getLogger(__name__)
This will specify in the logs which module the log came from, example:
INFO:__main__:Started
INFO:mylib:Doing something
INFO:__main__:Finished
There was a problem hiding this comment.
Yea that's a good idea. I will do that.
| self.logger = logging.get_logger("file_logging_tracker") | ||
| # Configure log level | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) |
There was a problem hiding this comment.
I don't think you even have to set log level for every file, you only have to set it once in the sft_trainer.py after parsing user's input right?
In each file, you just need to get a logger by doing:
import logging
logger = logging.getLogger(__name__)
and every time we log something, we just use that logger:
logger.info("Log this info")
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
tests/test_sft_trainer.py
Outdated
| assert training_args.log_level == "info" | ||
|
|
||
|
|
||
| @mock.patch.dict(os.environ, {}, clear=True) |
There was a problem hiding this comment.
Should include TRANSFORMERS_VERBOSITY to set the env and so you don't have to set it in the test and worry about removing it
| @mock.patch.dict(os.environ, {}, clear=True) | |
| @mock.patch.dict(os.environ, {"TRANSFORMERS_VERBOSITY": "info"}, clear=True) |
tests/test_sft_trainer.py
Outdated
| # Local | ||
| from tuning import sft_trainer | ||
| from tuning.config import configs, peft_config | ||
| from tuning.utils.logging import set_log_level |
There was a problem hiding this comment.
Since you moved the function that you are testing, you should also move the tests. They should be in /tests/utils/test_logging.py
tests/test_sft_trainer.py
Outdated
| assert training_args.log_level == "error" | ||
|
|
||
|
|
||
| @mock.patch.dict(os.environ, {}, clear=True) |
There was a problem hiding this comment.
similarly, want to include LOG_LEVEL here
| @mock.patch.dict(os.environ, {}, clear=True) | |
| @mock.patch.dict(os.environ, {"LOG_LEVEL": "info"}, clear=True) |
tests/test_sft_trainer.py
Outdated
| assert training_args.log_level == "passive" | ||
|
|
||
|
|
||
| @mock.patch.dict(os.environ, {}, clear=True) |
There was a problem hiding this comment.
Want to include LOG_LEVEL here to setup env
| @mock.patch.dict(os.environ, {}, clear=True) | |
| @mock.patch.dict(os.environ, {"LOG_LEVEL": "info"}, clear=True) |
| from tuning.trainercontroller.controllermetrics.metricshandler import MetricHandler | ||
|
|
||
| logger = logging.get_logger(__name__) | ||
| # Configure log level |
There was a problem hiding this comment.
remove comment, no longer relevant
There was a problem hiding this comment.
@anhuong Thanks for the quick review. I have made all changes mentioned above and pushed it again. Thank you!
As discussed, I can also wait for Dushyant's comment!
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
tests/utils/test_logging.py
Outdated
| from tuning.config import configs | ||
| from tuning.utils.logging import set_log_level | ||
|
|
||
| TRAIN_ARGS = configs.TrainingArguments( |
There was a problem hiding this comment.
Since this is same as the TRAIN_ARGS in sft_trainer, you can import this directly from the main test file instead of redeclaration, please see -
There was a problem hiding this comment.
Thank you for this input. This is done!
| super().__init__(name="aim", tracker_config=tracker_config) | ||
| self.logger = logging.get_logger("aimstack_tracker") | ||
| # Get logger with root log level | ||
| self.logger = logging.getLogger() |
There was a problem hiding this comment.
@Abhishek-TAMU any particular reason to remove module name from the logger I had kept it to segregate logs from various subcomponents.
There was a problem hiding this comment.
Same comment for file_logging_tracker
There was a problem hiding this comment.
Explained in the comment above..no need for futher explanation.
There was a problem hiding this comment.
Cool. Thank you!
There was a problem hiding this comment.
I still dont quite understand this. You're setting log_level at runtime to restrict the amount of logs that a user wants to see (debug vs info etc.). But if you don't do logging.get_logger("aimstack_tracker") and so on in each file, all the logs will say they come from root, not module specific, helpful but not as helpful if the module is also included.
Maybe it'd be clearer if you could share an example run of what the logs look like if user sets debug or error for example and we can see what the logs look like.
| @@ -0,0 +1,46 @@ | |||
| # Standard | |||
There was a problem hiding this comment.
Needs a code ownership declaration similar to this -
fms-hf-tuning/tuning/sft_trainer.py
Line 1 in d224be6
There was a problem hiding this comment.
This is done!
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
|
@anhuong Thank you again for all the multiple review on this PR. I have pushed the final changes. Feel free to have look whenever possible. Thanks! |
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
anhuong
left a comment
There was a problem hiding this comment.
Awesome work Abhishek! This will help a lot, many thanks!
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
aluu317
left a comment
There was a problem hiding this comment.
@Abhishek-TAMU Some comments inline. Thank you!
| or 'SFT_TRAINER_CONFIG_JSON_ENV_VAR'." | ||
| ) | ||
|
|
||
| # Configure log_level of python native logger. |
There was a problem hiding this comment.
Let's also add a comment here that CLI arg takes precedence over env var. And if neither is set, we use default "WARNING"
There was a problem hiding this comment.
Thank you! This is done
tests/utils/test_logging.py
Outdated
| def test_set_log_level_for_logger_with_env_var_and_cli(): | ||
| """ | ||
| Ensure that the correct log level is being set for python native logger and | ||
| transformers logger when env var LOG_LEVEL is used and CLI flag is passed |
There was a problem hiding this comment.
Can you add a quick comment that CLI arg takes precedence and we ignore env var LOG_LEVEL in this case?
There was a problem hiding this comment.
This is done!
| def set_log_level(train_args, logger_name=None): | ||
| """Set log level of python native logger and TF logger via argument from CLI or env variable. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Could you add an arg for logger_name here?
There was a problem hiding this comment.
This is done!
| super().__init__(name="aim", tracker_config=tracker_config) | ||
| self.logger = logging.get_logger("aimstack_tracker") | ||
| # Get logger with root log level | ||
| self.logger = logging.getLogger() |
There was a problem hiding this comment.
I still dont quite understand this. You're setting log_level at runtime to restrict the amount of logs that a user wants to see (debug vs info etc.). But if you don't do logging.get_logger("aimstack_tracker") and so on in each file, all the logs will say they come from root, not module specific, helpful but not as helpful if the module is also included.
Maybe it'd be clearer if you could share an example run of what the logs look like if user sets debug or error for example and we can see what the logs look like.
|
@aluu317 The reason behind removing |
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
Signed-off-by: Abhishek <maurya.abhishek@ibm.com>
|
Thank you @Abhishek-TAMU. This works! |


Description of the change
Aim was to move away from using logger modules from transformers and use python native logger. Now, log level is either set by CLI argument of
--log_levelOR environment variableTRANSFORMER_VERBOSITYORLOG_LEVEL. The setting of log level applies to both python native logger and log level of transformers logging.Related issue number
Closes #243
Closes #195
Closes #194
Closes #215
#994
How to verify the PR
LOG_LEVELORTRANSFORMER_VERBOSITYas desired log level and check if logs are correct according to passed value in env variable, by running tuning job for a small model likeMaykeye/TinyLLama-v0.Command:
log_levelvia cli argument, and check if logs are correct according to passed LOG_LEVEL, by running tuning job for a small model likeMaykeye/TinyLLama-v0.Command:
Was the PR tested