-
Notifications
You must be signed in to change notification settings - Fork 66
Fix: Removal of transformers logger and addition of python native logger #270
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
Changes from 15 commits
2b4ce8d
76fffc5
773f0d1
2e36147
8846bc2
4ed3878
efb8363
2ecfaf7
f2e8afb
c44b1dc
abd8abc
5d08efb
a4cc9c2
7fffda7
ba8a972
f1159f9
756d097
bf30ed2
cb846ca
da7acc6
8af5792
697056c
768d93a
ba489b5
dc9a521
f3a984a
024b12e
cfeb709
bf36b36
fe4b6d5
c544f47
f0fcfdb
e65cb2d
4841119
fe8bb05
5ecf4dd
89124ac
bddad06
0777460
3068f51
0866bce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,9 +61,6 @@ def get_base_model_from_adapter_config(adapter_config): | |
|
|
||
|
|
||
| def main(): | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) | ||
|
|
||
| if not os.getenv("TERMINATION_LOG_FILE"): | ||
| os.environ["TERMINATION_LOG_FILE"] = ERROR_LOG | ||
|
|
||
|
|
@@ -80,6 +77,14 @@ def main(): | |
| or 'SFT_TRAINER_CONFIG_JSON_ENV_VAR'." | ||
| ) | ||
|
|
||
| # Configure log_level of python native logger. | ||
| LOGLEVEL = None | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the PR review. This is done! |
||
| 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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 together
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done! Thank you for the input! |
||
| args = process_accelerate_launch_args(job_config) | ||
| logging.debug("accelerate launch parsed args: %s", args) | ||
| except FileNotFoundError as e: | ||
|
|
@@ -110,6 +115,10 @@ def main(): | |
| updated_args = serialize_args(job_config) | ||
| os.environ["SFT_TRAINER_CONFIG_JSON_ENV_VAR"] = updated_args | ||
|
|
||
| # Configure for Image to get log level as the code after | ||
| # launch_command only takes log level from env var LOG_LEVEL and not CLI | ||
| os.environ["LOG_LEVEL"] = LOGLEVEL | ||
|
|
||
| launch_command(args) | ||
| except subprocess.CalledProcessError as e: | ||
| # If the subprocess throws an exception, the base exception is hidden in the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| # Standard | ||
| import copy | ||
| import json | ||
| import logging | ||
| import os | ||
| import tempfile | ||
|
|
||
|
|
@@ -780,3 +781,61 @@ def test_pretokenized_dataset_wrong_format(): | |
| # is essentially swallowing a KeyError here. | ||
| with pytest.raises(ValueError): | ||
| sft_trainer.train(MODEL_ARGS, data_args, train_args, PEFT_PT_ARGS) | ||
|
|
||
|
|
||
| def test_set_log_level_for_logger_default(): | ||
| """ | ||
| Ensure that the correct log level is being set | ||
| for python native logger and transformers logger | ||
| """ | ||
|
|
||
| # Set env var TRANSFORMERS_VERBOSITY as None and test | ||
| os.unsetenv("TRANSFORMERS_VERBOSITY") | ||
| os.unsetenv("LOG_LEVEL") | ||
| os.unsetenv("SFT_TRAINER_CONFIG_JSON_ENV_VAR") | ||
| os.unsetenv("SFT_TRAINER_CONFIG_JSON_PATH") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another way you can do this is by mocking the env: # decorator can be used on whole function
@mock.patch.dict(os.environ, {}, clear=True)
# or can create context scope
with mock.patch.dict(os.environ, {}, clear=True):
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! |
||
| train_args = copy.deepcopy(TRAIN_ARGS) | ||
|
|
||
| # TEST IF NO ENV VAR ARE SET AND NO CLI ARGUMENT IS PASSED | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't need this comment since you should have described it both in the test name and in the test docstring. thus you should include a little more in the docstring to say "Ensure that the correct log level is being set for python native logger and transformers logger when no env var or CLI flag is passed"
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Thanks! |
||
| training_args, logger = sft_trainer.set_log_level(train_args) | ||
| assert logger.level == logging.WARNING | ||
| assert training_args.log_level == "warning" | ||
|
|
||
|
|
||
| def test_set_log_level_for_logger_with_env_var(): | ||
| """ | ||
| Ensure that the correct log level is being set | ||
| for python native logger and transformers logger | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should be more descriptive per test case and explain how the tests differentiate from each other. For example, this one should say "Ensure that the correct log level is being set for python native logger and transformers logger when LOG_LEVEL env var used"
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
| """ | ||
|
|
||
| os.unsetenv("TRANSFORMERS_VERBOSITY") | ||
| os.unsetenv("LOG_LEVEL") | ||
| os.unsetenv("SFT_TRAINER_CONFIG_JSON_ENV_VAR") | ||
| os.unsetenv("SFT_TRAINER_CONFIG_JSON_PATH") | ||
| train_args = copy.deepcopy(TRAIN_ARGS) | ||
|
|
||
| # TEST IF LOG_LEVEL ENV VAR IS SET AND NO CLI ARGUMENT IS PASSED | ||
| os.environ["LOG_LEVEL"] = "info" | ||
| training_args, logger = sft_trainer.set_log_level(train_args) | ||
| assert logger.level == logging.INFO | ||
| assert training_args.log_level == "info" | ||
|
|
||
|
|
||
| def test_set_log_level_for_logger_with_env_var_and_cli(): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add another test case where:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if I understand the case completely on what we want to test here. Hence to save iteration, sharing the test case here, for your input and comments, Thanks! |
||
| """ | ||
| Ensure that the correct log level is being set | ||
| for python native logger and transformers logger | ||
| """ | ||
|
|
||
| os.unsetenv("TRANSFORMERS_VERBOSITY") | ||
| os.unsetenv("LOG_LEVEL") | ||
| os.unsetenv("SFT_TRAINER_CONFIG_JSON_ENV_VAR") | ||
| os.unsetenv("SFT_TRAINER_CONFIG_JSON_PATH") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update per previous comments |
||
| train_args = copy.deepcopy(TRAIN_ARGS) | ||
|
|
||
| # TEST IF LOG_LEVEL ENV VAR IS SET AND --log_level CLI ARGUMENT IS PASSED | ||
| os.environ["LOG_LEVEL"] = "info" | ||
| train_args.log_level = "error" | ||
| training_args, logger = sft_trainer.set_log_level(train_args) | ||
| assert logger.level == logging.ERROR | ||
| assert training_args.log_level == "error" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,13 @@ class TrainingArguments(transformers.TrainingArguments): | |
| + "Requires additional configs, see tuning.configs/tracker_configs.py" | ||
| }, | ||
| ) | ||
| log_level: str = field( | ||
| default="passive", | ||
| metadata={ | ||
| "help": "The log level to adopt during training. \ | ||
| Possible values are 'debug', 'info', 'warning', 'error' and 'critical'" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add note on what "passive" value means
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added note in metadata! |
||
| }, | ||
| ) | ||
|
|
||
|
|
||
| @dataclass | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,8 @@ | |||||
| from typing import Dict, List, Optional, Union | ||||||
| import dataclasses | ||||||
| import json | ||||||
| import logging | ||||||
| import os | ||||||
| import sys | ||||||
| import time | ||||||
| import traceback | ||||||
|
|
@@ -33,7 +35,7 @@ | |||||
| LlamaTokenizerFast, | ||||||
| TrainerCallback, | ||||||
| ) | ||||||
| from transformers.utils import is_accelerate_available, logging | ||||||
| from transformers.utils import is_accelerate_available | ||||||
| from trl import SFTConfig, SFTTrainer | ||||||
| import fire | ||||||
| import transformers | ||||||
|
|
@@ -111,8 +113,6 @@ def train( | |||||
| fused_lora and fast_kernels must used together (may change in future). \ | ||||||
| """ | ||||||
|
|
||||||
| logger = logging.get_logger("sft_trainer") | ||||||
|
|
||||||
| # Validate parameters | ||||||
| if (not isinstance(train_args.num_train_epochs, (float, int))) or ( | ||||||
| train_args.num_train_epochs <= 0 | ||||||
|
|
@@ -218,9 +218,9 @@ def train( | |||||
| ) | ||||||
|
|
||||||
| max_seq_length = min(train_args.max_seq_length, tokenizer.model_max_length) | ||||||
| logger.info("Max sequence length is %s", max_seq_length) | ||||||
| logging.info("Max sequence length is %s", max_seq_length) | ||||||
| if train_args.max_seq_length > tokenizer.model_max_length: | ||||||
| logger.warning( | ||||||
| logging.warning( | ||||||
| "max_seq_length %s exceeds tokenizer.model_max_length \ | ||||||
| %s, using tokenizer.model_max_length %s", | ||||||
| train_args.max_seq_length, | ||||||
|
|
@@ -231,16 +231,16 @@ def train( | |||||
| # TODO: we need to change this, perhaps follow what open instruct does? | ||||||
| special_tokens_dict = {} | ||||||
| if tokenizer.pad_token is None: | ||||||
| logger.warning("PAD token set to default, missing in tokenizer") | ||||||
| logging.warning("PAD token set to default, missing in tokenizer") | ||||||
| special_tokens_dict["pad_token"] = configs.DEFAULT_PAD_TOKEN | ||||||
| if tokenizer.eos_token is None: | ||||||
| logger.warning("EOS token set to default, missing in tokenizer") | ||||||
| logging.warning("EOS token set to default, missing in tokenizer") | ||||||
| special_tokens_dict["eos_token"] = configs.DEFAULT_EOS_TOKEN | ||||||
| if tokenizer.bos_token is None: | ||||||
| logger.warning("BOS token set to default, missing in tokenizer") | ||||||
| logging.warning("BOS token set to default, missing in tokenizer") | ||||||
| special_tokens_dict["bos_token"] = configs.DEFAULT_BOS_TOKEN | ||||||
| if tokenizer.unk_token is None: | ||||||
| logger.warning("UNK token set to default, missing in tokenizer") | ||||||
| logging.warning("UNK token set to default, missing in tokenizer") | ||||||
| special_tokens_dict["unk_token"] = configs.DEFAULT_UNK_TOKEN | ||||||
|
|
||||||
| # TODO: lower priority but understand if resizing impacts inference quality and why its needed. | ||||||
|
|
@@ -254,11 +254,11 @@ def train( | |||||
|
|
||||||
| # Configure the collator and validate args related to packing prior to formatting the dataset | ||||||
| if train_args.packing: | ||||||
| logger.info("Packing is set to True") | ||||||
| logging.info("Packing is set to True") | ||||||
| data_collator = None | ||||||
| packing = True | ||||||
| else: | ||||||
| logger.info("Packing is set to False") | ||||||
| logging.info("Packing is set to False") | ||||||
| packing = False | ||||||
|
|
||||||
| # Validate if data args are set properly | ||||||
|
|
@@ -323,7 +323,7 @@ def train( | |||||
| tracker.track(metric=v, name=k, stage="additional_metrics") | ||||||
| tracker.set_params(params=exp_metadata, name="experiment_metadata") | ||||||
| except ValueError as e: | ||||||
| logger.error( | ||||||
| logging.error( | ||||||
| "Exception while saving additional metrics and metadata %s", | ||||||
| repr(e), | ||||||
| ) | ||||||
|
|
@@ -461,12 +461,46 @@ def parse_arguments(parser, json_config=None): | |||||
| ) | ||||||
|
|
||||||
|
|
||||||
| def main(**kwargs): # pylint: disable=unused-argument | ||||||
| logger = logging.get_logger("__main__") | ||||||
| def set_log_level(parsed_training_args): | ||||||
| """Set log level of python native logger and TF logger via argument from CLI or env variable. | ||||||
|
|
||||||
| Args: | ||||||
| parsed_training_args | ||||||
| Training arguments for training model. | ||||||
| """ | ||||||
|
|
||||||
| # Clear any existing handlers if necessary | ||||||
| for handler in logging.root.handlers[:]: | ||||||
| logging.root.removeHandler(handler) | ||||||
|
|
||||||
| # Configure Python native logger log level | ||||||
| # If CLI arg is passed, assign same log level to python native logger | ||||||
| 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() | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| logging.basicConfig(level=LOGLEVEL) | ||||||
| train_logger = logging.getLogger() | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you call this on line 497, you don't need to call it within this block as well
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this work to attain the logic we're looking for?
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()
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't add this line Also I think in your code above in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's chat about this logic to clarify -- update: done! |
||||||
|
|
||||||
| # 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: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can do
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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()
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we are required to check "TRANFORMERS_VERBOSITY". Hence I haven't added this code in final commit. |
||||||
|
|
||||||
| # Check if "--log_level" CLI argument is not used (passive/warning is the default log level) | ||||||
| if parsed_training_args.log_level == "passive": | ||||||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||||||
|
|
||||||
| # Set log_level in TrainingArguments | ||||||
| parsed_training_args.log_level = LOGLEVEL.lower() | ||||||
|
|
||||||
| train_logger = logging.getLogger() | ||||||
| return parsed_training_args, train_logger | ||||||
|
|
||||||
|
|
||||||
| def main(**kwargs): # pylint: disable=unused-argument | ||||||
| parser = get_parser() | ||||||
| job_config = get_json_config() | ||||||
| logger.debug("Input args parsed: %s", job_config) | ||||||
| # accept arguments via command-line or JSON | ||||||
| try: | ||||||
| ( | ||||||
|
|
@@ -481,7 +515,11 @@ def main(**kwargs): # pylint: disable=unused-argument | |||||
| fusedops_kernels_config, | ||||||
| exp_metadata, | ||||||
| ) = parse_arguments(parser, job_config) | ||||||
| logger.debug( | ||||||
|
|
||||||
| # Function to set log level for python native logger and transformers training logger | ||||||
| training_args, _ = set_log_level(training_args) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you not using the 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. # named logger if you do getLogger("sft_trainer_train")
WARNING:sft_trainer_train:My warning
# root logger if you do getLogger()
WARNING:root:My warning
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
|
||||||
| logging.debug( | ||||||
| "Input args parsed: \ | ||||||
| model_args %s, data_args %s, training_args %s, trainer_controller_args %s, \ | ||||||
| tune_config %s, file_logger_config, %s aim_config %s, \ | ||||||
|
|
@@ -511,12 +549,12 @@ def main(**kwargs): # pylint: disable=unused-argument | |||||
| try: | ||||||
| metadata = json.loads(exp_metadata) | ||||||
| if metadata is None or not isinstance(metadata, Dict): | ||||||
| logger.warning( | ||||||
| logging.warning( | ||||||
| "metadata cannot be converted to simple k:v dict ignoring" | ||||||
| ) | ||||||
| metadata = None | ||||||
| except ValueError as e: | ||||||
| logger.error( | ||||||
| logging.error( | ||||||
| "failed while parsing extra metadata. pass a valid json %s", repr(e) | ||||||
| ) | ||||||
|
|
||||||
|
|
@@ -539,27 +577,27 @@ def main(**kwargs): # pylint: disable=unused-argument | |||||
| fusedops_kernels_config=fusedops_kernels_config, | ||||||
| ) | ||||||
| except (MemoryError, OutOfMemoryError) as e: | ||||||
| logger.error(traceback.format_exc()) | ||||||
| logging.error(traceback.format_exc()) | ||||||
| write_termination_log(f"OOM error during training. {e}") | ||||||
| sys.exit(INTERNAL_ERROR_EXIT_CODE) | ||||||
| except FileNotFoundError as e: | ||||||
| logger.error(traceback.format_exc()) | ||||||
| logging.error(traceback.format_exc()) | ||||||
| write_termination_log("Unable to load file: {}".format(e)) | ||||||
| sys.exit(USER_ERROR_EXIT_CODE) | ||||||
| except HFValidationError as e: | ||||||
| logger.error(traceback.format_exc()) | ||||||
| logging.error(traceback.format_exc()) | ||||||
| write_termination_log( | ||||||
| f"There may be a problem with loading the model. Exception: {e}" | ||||||
| ) | ||||||
| sys.exit(USER_ERROR_EXIT_CODE) | ||||||
| except (TypeError, ValueError, EnvironmentError) as e: | ||||||
| logger.error(traceback.format_exc()) | ||||||
| logging.error(traceback.format_exc()) | ||||||
| write_termination_log( | ||||||
| f"Exception raised during training. This may be a problem with your input: {e}" | ||||||
| ) | ||||||
| sys.exit(USER_ERROR_EXIT_CODE) | ||||||
| except Exception as e: # pylint: disable=broad-except | ||||||
| logger.error(traceback.format_exc()) | ||||||
| logging.error(traceback.format_exc()) | ||||||
| write_termination_log(f"Unhandled exception during training: {e}") | ||||||
| sys.exit(INTERNAL_ERROR_EXIT_CODE) | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,11 +15,11 @@ | |
| # Standard | ||
| from datetime import datetime | ||
| import json | ||
| import logging | ||
| import os | ||
|
|
||
| # Third Party | ||
| from transformers import TrainerCallback | ||
| from transformers.utils import logging | ||
|
|
||
| # Local | ||
| from .tracker import Tracker | ||
|
|
@@ -80,7 +80,10 @@ def __init__(self, tracker_config: FileLoggingTrackerConfig): | |
| which contains the location of file where logs are recorded. | ||
| """ | ||
| super().__init__(name="file_logger", tracker_config=tracker_config) | ||
| self.logger = logging.get_logger("file_logging_tracker") | ||
| # Configure log level | ||
| LOGLEVEL = os.environ.get("LOG_LEVEL", "WARNING").upper() | ||
| logging.basicConfig(level=LOGLEVEL) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a few of these to fix, not going to comment in each file
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: and every time we log something, we just use that
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be helpful also if in each file, you could do: This will specify in the logs which module the log came from, example:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea that's a good idea. I will do that. |
||
| self.logger = logging.getLogger("file_logging_tracker") | ||
|
|
||
| def get_hf_callback(self): | ||
| """Returns the FileLoggingCallback object associated with this tracker. | ||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is done