mlflow logging integration with yolox training #1773
Conversation
Im-Himanshu
changed the title
mlflow integration with yolox training
|
Tested the logging on to data bricks, following logs are available for all the runs. Logged params
logged metrices
logged artifacts
|
|
@FateScript Requesting you to please review the pull request. |
There was a problem hiding this comment.
@Im-Himanshu Thanks for your contribution : )
Please check my review suggestion and lint your code to pass the github workflow.
|
Any update? @Im-Himanshu |
@FateScript Excuse me for the delayed response, I have pushed new commits to implement all the suggestions. |
@FateScript Gentle Reminder to Please check and merge the request. |
yolox/utils/logger.py
Outdated
| @@ -98,8 +98,11 @@ def setup_logger(save_dir, distributed_rank=0, filename="log.txt", mode="a"): | |||
|
|
|||
| logger.remove() | |||
| save_file = os.path.join(save_dir, filename) | |||
| crnt_log_save_file = os.path.join(save_dir, 'train_log_crnt.txt') | |||
There was a problem hiding this comment.
Why this train_log_crnt.txt is needed? Seems that your code redirect io to it and remove this file if it exists.
There was a problem hiding this comment.
@FateScript As you have suggested in your earlier review that you recommend creating a new logger file.
Moreover, it is deleted at that start to keep only current run logs in this and upload that part only, current .log file has logs of all the previous runs which may be confusing in experiment tracking.
There was a problem hiding this comment.
It's my bad, I didn't make it clear. In fact, the logger file means logger.py but not the file where logs are saved.
Your code here just make a copy of the log file. It's better for you to reset to code here. Thanks!
There was a problem hiding this comment.
Removed it and reverted to old code.
though I think it would have been better to log only the current experiment logs to mlflow.
|
@Im-Himanshu Also please don't forget to lint your code. |
Linted the code, the only major issue in lint, is the import statement which has to be done inside class (same as being done in wandb logger) because this is an optional feature. |
|
@FateScript @Cloudhax23 completed all the suggestion, please check. |
yolox/utils/logger.py
Outdated
| @@ -98,8 +98,11 @@ def setup_logger(save_dir, distributed_rank=0, filename="log.txt", mode="a"): | |||
|
|
|||
| logger.remove() | |||
| save_file = os.path.join(save_dir, filename) | |||
| crnt_log_save_file = os.path.join(save_dir, 'train_log_crnt.txt') | |||
There was a problem hiding this comment.
It's my bad, I didn't make it clear. In fact, the logger file means logger.py but not the file where logs are saved.
Your code here just make a copy of the log file. It's better for you to reset to code here. Thanks!
|
@FateScript Removed the additional logger and linted the code again to remove build process error. |
|
@Im-Himanshu please isort your code( |
@FateScript in I think my isort setting are not matched to the project setting, if there are any settings of isort that I can follow for this project, I can rerun the isort or otherwise the sort that I have done now should be correct based on my intuition. |
yolox/utils/mlflow_logger.py
Outdated
| from yolox.utils import is_main_process | ||
|
|
||
|
|
||
|
|
If you instal the correct isort version(4.3.21 here), everything will be ok. |
@FateScript My isort version was not as required by the library, hence the previous errors. |
|
LGTM. |
Needed integration of
yoloxto log experiments with mlflow, the pull request provide additional option in -l --logger argument to log output in "mlflow".Requires an environment file (
.env) in the root folder of the projects.required additional dependency of
mlflowand python-dotenv failing which error is raised if logger is set to mlflow.