Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to pass number of workers to torch dataloaders #456

Open
RaczeQ opened this issue Jun 7, 2024 · 2 comments
Open

Add option to pass number of workers to torch dataloaders #456

RaczeQ opened this issue Jun 7, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@RaczeQ
Copy link
Collaborator

RaczeQ commented Jun 7, 2024

Currently dataloaders don't use any kwargs and work in single-threaded mode. Add option to use not only **trainer_kwargs but also **dataloader_kwargs.

@RaczeQ RaczeQ added the enhancement New feature or request label Jun 7, 2024
@sabman
Copy link
Contributor

sabman commented Aug 31, 2024

Quick question about this. Do we want to maintain backwards compatibility with a deprecation warning or shall we change the API to change the constructor signature by e.g. removing batch_size as an argument to dataloader_kwargs. (See some initial work here #464)

My personal take on it is to maintain backwards compatibility and have deprication warnings now with a breaking release later using semantic versioning.

Happy to take your lead on this.

@simonusher
Copy link
Collaborator

Hi @sabman! First of all, thank you for working on this.

As for maintaining backwards compatibility, it hasn't been our top priority, but we haven't fully ignored it either. I think we might have had at least a couple of breaking changes on minor versions as "this is new, we're moving fast and things may break". Mostly because we didn't know how people would use the library (and they'd want to use it), and if the API we had designed made sense.
But if you're willing to add a deprecation warning then we'd appreciate this and we can maintain it up to the next major. Imo it's not a huge change, so we can do without it.

So in short: if you feel it's worth to add a deprecation warning then go for it, we'll appreciate it. If not then we can release the next minor with it, not fully following semantic versioning yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants