Make output_dir Optional in TrainingArguments #27866
#35735
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
January 16, 2025 22:32
sambhavnoobcoder
requested review from
Rocketknight1 and
ArthurZucker
as code owners
|
cc @SunMarc @muellerzr for Trainer! |
SunMarc
approved these changes
Jan 20, 2025
There was a problem hiding this comment.
I don't mind making it optional ! Left a comment. Can you have a second look @muellerzr ? Also call make style to fix the CI !
src/transformers/training_args.py
Outdated
Comment on lines
231
to
232
| output_dir (`str`, *optional*): | ||
| The output directory where the model predictions and checkpoints will be written. |
There was a problem hiding this comment.
Please update the description to include the default value for output_dir. We can maybe call it trainer_output WDYT ?
There was a problem hiding this comment.
yes i agree , trainer_output sounds more intitutive .I've made that change as well as updated the descriptions in 7831474 commit .
Also , the PR is now clean and ready for merging .
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses an issue where specifying an
output_dirwas mandatory inTrainingArguments. This behavior could be confusing or cumbersome in scenarios where users do not necessarily need a dedicated output directory or prefer to let the library handle default paths.By making
output_diroptional and defaulting to a temporary directory (named"tmp_trainer") when not specified, we simplify the API usage. If a user later needs to track outputs in a dedicated directory, they can still provide a valid path.Fixes : #27866
Proposed Solution
• Allow
output_dirto be set toNone.• Default to a built-in location (e.g.,
"tmp_trainer") ifNoneis provided.Implementation
The changes introduce a check to automatically assign a default directory if none is specified. This preserves backward compatibility for users already specifying
output_dirwhile improving the experience for those who prefer a simpler workflow.Testing
• Added a test covering the scenario when
output_diris not provided.• Verified existing tests using a custom
output_dirare unaffected.• Extended tests to ensure the directory is only created when needed (depending on the save strategy).
Screenshots
Below is a screenshot of all testcases passing :
cc : @ArthurZucker @Rocketknight1