ArneBinder / pytorch-ie

PyTorch-IE: State-of-the-art Information Extraction in PyTorch
MIT License
75 stars 7 forks source link

Remove datamodule #181

Closed ChristophAlt closed 1 year ago

ChristophAlt commented 2 years ago

I was never a fan of the whole datamodule concept and it adds way to much complexity for way to little gain. I'm confident our users are able two create two / three datasets and the respective dataloaders on their own.

ArneBinder commented 2 years ago

OK, but can we move that to the template?

ChristophAlt commented 2 years ago

No, it should be removed completely. As Gabriel pointed out (see here: https://github.com/GabrielKP/svo/issues/8#issuecomment-1108613369) it's too confusing and that also resonates with the feeling I had from the start. The goal of PIE was never to make everything a super easy one liner but to be as explicit as possible while abstracting only the basic building blocks. The datamodule is already one indirection too much.

ArneBinder commented 2 years ago

But as I understand it, the idea of the original template is really to have sth that works from the beginning and which the user then adapts for their needs. In the original template, this is the mnist example. As described in the readme, you can call

python train.py

or

python train.py experiment=example

and it works. However, this would require any kind of DataModule (see MNISTDataModule in the original template).

ChristophAlt commented 2 years ago

I don't understand this obsession with the datamodule. It's a completely optional concept, even in pytorch-lightning, and I never understood why people use it in the first place. It's so much more convoluted compared to just load your dataset, put every split into a dataloader, and pass it to trainer. Done. This is clear and straightforward, and everyone who has ever used pytorch is immediately familiar with the concept. Edit: Also the goal was never to stay as close as possible to the original template. I don't think too many people are familiar with that template anyways, even though it's quite popular. The goal of PIE is to provide basic building blocks and facilitate maximum freedom, not minimal lines of code. And in my opinion every new concept (e.g. datamodule, bridge, etc.) makes it just more difficult to adapt and understand. Now one might argue why the TaskModule exists and why this is not part of the Model. That's easily answered, models are just much more generic and thus can typically be shared among many tasks, e.g. a text classification model can be applied to many tasks without any change, e.g. relation classification, sentiment analysis, topic classification, you name it. But preparing inputs and outputs, and assigning annotations to a document differs considerable among tasks.

ArneBinder commented 2 years ago

This was not about having a one-liner, but a minimal example that works out of the box. I find sth like this very useful and I'm quite sure that I'm not the only one. And I'm not obsessed with DataModules, if there are better ways, I'm very fine without it. I just thought that is the way to go, but I'm really not so experienced in these technicalities.