Refactor get_XXX_dataloader from Trainer#38090
Conversation
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the |
|
cc @SunMarc! |
There was a problem hiding this comment.
Thanks for the proposition @yaswanth19, since this is something that was here for a long time, removing this can actually be breaking for some code that doesn't process the train / test / eval dataset the same way. We can definitely make it easier to maintain by defining some utility functions but as for removing these methods, I prefer not to do that. Also get_eval_dataloader is bit different from get_test_dataloader
Thanks @SunMarc , I do agree that this might be breaking change for some other codebases. I don't see a scenario where I expect my eval_dataloader and test_dataloader to behave different and the core logic seems to be same to me 😅 . Probably we can keep get_test_dataloader but use get_eval_dataloader inside to simplify th redundant logic. WDYT? |
|
In any case, we will keep both functions. I don't think this make sense to use get_eval_dataloader inside get_test_dataloader, maybe you can create a |
get_test_dataloader from Trainerget_XXX_dataloader from Trainer
|
@SunMarc Please review 🤗 . I haven't added any test as IMO those should be covered when we test get_XX_dataloader. |
|
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. |
SunMarc
left a comment
There was a problem hiding this comment.
Thanks a lot for this refactor ! This is much better !
* Remove test_dataloader * refactor
* Remove test_dataloader * refactor
What does this PR do?
Refactors dataloader functions in Trainer