search

uclnlp / jack

Jack the Reader
MIT License
257 stars 82 forks source link

Move `jack/train_reader.py` into `bin/jack-cli.py` #277

Closed pminervini closed 6 years ago

pminervini commented 7 years ago

train_reader.py is a CLI interface for Jack's functionalities - I think it can make sense to keep it separate from the framework.

Do you think so as well?

dirkweissenborn commented 7 years ago

I think train just supports train right? We could make it a cli but it currently isn't as far as I understand it.

dirkweissenborn commented 7 years ago

Anyway, this should not happen before v0.1

pminervini commented 7 years ago

jack/train_reader.py is how we interact with Jack from the command line - I think that to keep it separated from the rest of the code base helps wrt. clarity

dirkweissenborn commented 7 years ago

So it might live in something like cli/train_reader.py or bin/train_reader.py. That would be ok with me

dirkweissenborn commented 7 years ago

I mean, I just want to have a script that says train. I think we don't have too many cli use cases so we can as well make them explicit.

pminervini commented 7 years ago

Ok! :) I'll handle this right after Issue #274

riedelcastro commented 7 years ago

Generally, I like the idea of separating this. However, I think right now the train_reader script has too much re-usable training logic inside to be just an "interface". What I would like to see is a train_reader function in the framework that implements most of the training logic (early stopping etc), and which I call through an API. Something like

def train_reader(reader, train_data, test_data, dev_data, configuration:dict)

(with more type specifications). The CLI interface should then be a very light wrapper around this function (which itself still lives in the framework and can be accessed programmatically).