Fix all output_dir in test_trainer.py to use tmp_dir #35266
Conversation
nhamanasu
changed the title
| def test_optim_supported(self, optim: str, expected_cls, expected_kwargs): | ||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| trainer = get_regression_trainer(output_dir=tmp_dir, optim=optim) | ||
|
|
||
| trainer = get_regression_trainer(**training_args.to_dict()) | ||
| trainer.train() | ||
| # exercises all the valid --optim options | ||
| self.check_optim_and_kwargs(trainer.args, expected_cls, expected_kwargs) | ||
| trainer.train() |
There was a problem hiding this comment.
Very nice. Thank you for working on this ❤️ @nhamanasu.
Only a few small nits. Let me know if you want to change accordingly.
tests/trainer/test_trainer.py
Outdated
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| for mix_bf16 in [True, False]: |
There was a problem hiding this comment.
I would probably reverse the order of these 2 lines
tests/trainer/test_trainer.py
Outdated
| for name, param in tiny_llama.named_parameters(): | ||
| self.assertFalse(torch.allclose(param, previous_params[name].to(param.device), rtol=1e-12, atol=1e-12)) |
There was a problem hiding this comment.
the extra indent is not really necessary, but OK to keep this changes
tests/trainer/test_trainer.py
Outdated
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| for mix_bf16 in [True, False]: |
There was a problem hiding this comment.
better to revert order of 2 lines
tests/trainer/test_trainer.py
Outdated
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| for mix_bf16 in [True, False]: |
|
Thanks! One more nits: there is no content in After this last point, I will merge |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Thanks! I'm watching the control characters and whitespaces, but I couldn't find any difference. |
|
I copied .gitignore from newly cloned transformer repo's main branch, and somehow it fixed the unintentional diff! Maybe it's related to LF/CRLF or file mode, but I'm not sure in the end... |
|
no worry, will merge when CI fishing. Thanks again! |
|
Only one test file changed in this PR and the change logic is not involving any real trainer's logic. Will merge |
What does this PR do?
Split from the PR: #35243
Current
test_trainer.pyhas so many direct specifications ofoutpu_dirto TrainingArguments.I've replaced all of them with tmp_dir so that no unnecessary folders will be made under root dir.
The targets are:
Just a random thought
The current CircleCI doesn't seem to include tests for trainer. Is it as intended?
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@ydshieh @SunMarc