CUNY-CL / yoyodyne

Small-vocabulary sequence-to-sequence generation with optional feature conditioning
Apache License 2.0
25 stars 15 forks source link

Python interface for loading #107

Closed Adamits closed 11 months ago

Adamits commented 11 months ago

Previously, I recall that we had methods like get_trainer and get_model to go along with train.get_trainer_from_argparse_args and train.get_model_from_argparse_args.

These seem to have disappeared. What is the rationale behind that? Was this a mistake?

kylebgorman commented 11 months ago

I sort of put them in there anticipating having tests in place not long after, but never got around to finishing the testing, and the interfaces kept changing in the meantime. I think I should complete #91 and we should complete #60 in general before we are ready to freeze this interface.

Adamits commented 11 months ago

Cool! So

before we are ready to freeze this interface.

The interface being the exact argument names? And then we would just add the other method, which would share that interface?

kylebgorman commented 11 months ago

By interface I mean the helper functions to create the pieces you need for training. Maybe something like:

def get_datamodule(...): ...

def get_model(...): ...

def get_trainer(...): ...

You want different flavors of these in train and predict modules.

Alternatively we could have a sort of "god class" that wraps the trainer object.

Adamits commented 11 months ago

Ahhh got. Makes sense.

Alternatively we could have a sort of "god class" that wraps the trainer object.

And this class would have the helpers as method?

kylebgorman commented 11 months ago

Alternatively we could have a sort of "god class" that wraps the trainer object.

And this class would have the helpers as method?

That's what I had in mind. Though I am not sure if that's a good idea vs. the interface I outlined above.

Adamits commented 11 months ago

Yeah it seems clean, but then, I would also want to advocate for exposing the PTL trainer as much as possible (not totally sure whether either suggestion is better than the other to this end).

kylebgorman commented 11 months ago

Arguably we have a reasonably stable API for this as of #110. Do you have particular suggestions for what else is needed?

kylebgorman commented 11 months ago

Going to close this---I think we have a reasonably good interface now. Feel free to reopen with specific requests.