[misc] feat: Update file logger path output to absolute path#5924
[misc] feat: Update file logger path output to absolute path#5924vermouth1992 wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the file logger initialization to display the absolute path of the log file. However, a critical issue was identified where the directory for the log file is not guaranteed to exist if the path is provided via environment variables, which could lead to a FileNotFoundError. It is recommended to ensure the directory is created using os.makedirs(os.path.dirname(self.filepath), exist_ok=True) before opening the file.
| os.makedirs(directory, exist_ok=True) | ||
| self.filepath = os.path.join(directory, f"{self.experiment_name}.jsonl") | ||
| print(f"Creating file logger at {self.filepath}") | ||
| print(f"Creating file logger at {os.path.abspath(self.filepath)}") |
There was a problem hiding this comment.
While this change correctly makes the log message more informative, it highlights a pre-existing critical bug. If VERL_FILE_LOGGER_PATH is set from the environment, the directory for the specified path is not created, as os.makedirs is only called when the path is generated by the program. This will lead to a FileNotFoundError on line 267 if the directory doesn't exist.
The directory for self.filepath should be created before the file is opened, regardless of how the path is specified. A possible fix is to call os.makedirs(os.path.dirname(self.filepath), exist_ok=True) after self.filepath is determined.
There was a problem hiding this comment.
While this change correctly makes the log message more informative, it highlights a pre-existing critical bug. If
VERL_FILE_LOGGER_PATHis set from the environment, the directory for the specified path is not created, asos.makedirsis only called when the path is generated by the program. This will lead to aFileNotFoundErroron line 267 if the directory doesn't exist.The directory for
self.filepathshould be created before the file is opened, regardless of how the path is specified. A possible fix is to callos.makedirs(os.path.dirname(self.filepath), exist_ok=True)afterself.filepathis determined.
Could you propose a fix
What does this PR do?
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,veomni,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,fully_async,one_step_off,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)recipesubmodule, please also update the reference to the submodule commit viagit submodule update --remoteorcd recipe && git pull origin main.