Improve feature embeddings implementation

[flauted]

Per discussion with @bpopeters and @vince62s on #1196

In the long run, by the time [data is] batched it shouldn't be necessary to know the data type. It's just a batch of data that the model should be able to handle. It only comes up right now because our way of handling feature embeddings (a feature almost no one uses) is super hacky.

The question is just when, where, and how they get numericalized. I think the best way would be some kind of multilevel field that takes an input which is words plus arbitrarily many features and when numericalized produced the sort of stacked tensor that the embeddings module expects. The NestedField in torchtext almost, but not quite, fits the bill.

It would save the trainer and translator from having to reason about the types of the source and target data.

I made a first pass at designing a multilevel field that handles the batching of features so that it's not in inputters.make_features. I opened a PR from that branch against my branch for #1196 so that you can review the diff and guide the design further before I try to open one here (also because #1196 isn't merged). It's here. I checked the PR script and all seems well.

[vince62s]

I merged #1196 but then I had to revert it.
The reason is that since the introduction of _feature_tokenize in #1132 (14 days ago), the model embeds it.
So all training of new model over the past 14 days include this with a path of inputter.inputter.py
So if we reclass it, it's a breaking change, just for a window of 14 days.

any insight ?

[flauted]

Yeah I see the problem. I was confused that it wasn't backwards compatible because I thought the pull_request_chk.sh tested against legacy models. Now that you say 14 days it makes sense.

I opened an issue for the backwards compatibility on my fork: flauted#6

Just importing _feature_tokenize in inputter.py (and doing nothing with it - yuck) resolves your specific problem.

Moving make_img causes a similar problem with img models. Again adding from onmt.inputters.image_dataset import batch_img as make_img to inputter.py fixes.

Now, the audio AudioSeqField/removing src_lengths is a more fundamental problem. I completely removed make_audio in the process of getting rid of src_lengths. Moreover, somewhere in the process of loading the checkpoint, there would need to be a check to see if it contains an audio model and if so the Field needs updated to an AudioSeqField.

There would need to be something similar for the MultiLevelField in this issue.

Unfortunately, since ModelSaver includes fields in the checkpoint, being backwards compatible/being able to unpickle those checkpoints forces a lot of rigidity on the API.

[flauted]

Well I don't know what kind of release cycle you guys are on but if it's at all possible to make these data type simplifications/field subclasses, accept that they're breaking, and bump up the version, I think that's the best way to go rather than write a bunch of kludgy backwards compatibility stuff. What do you think?

[vince62s]

for the 14 day gap, it's not a big deal, we can always say that v0.7 was the last release.
_feature_tokenize outside inputters should be fine.
For img I am less familiar with the issue and whether it has changed vs pre #1132

Last point on audio: well unsure about consequences. But travis passes, so it seems compatible with old pretrained models.

[vince62s]

@da03 we are making as much effort as we can to maintain some backwards compatibility with Text models, (IMG too). Audio could be an issue if we want to really improve the API. IMO, there are not so many people using onmt-py for audio work, it's more a showcase than anything else.
Do you have an opinion on this ?

[da03]

Agreed, I think improving API should come at a higher priority than making audio models compatible. I can always retrain the audio models to make the travis tests pass.

[flauted]

None of this comes up with old style vocabs because the fields (including the pre/post-processing) aren't pickled. The saving/loading in 0.7.0 lets us refactor and subclass Fields as necessary and only worry about loading the vocab back into the right place in the field. That's much more refactor-able than saving fields. But that breaks compatibility with checkpoints that save fields, meaning (if I understand correctly) any checkpoints since #1132.

[flauted]

If dropping compatibility for the last 14 days is okay, then reverting #1203 or reopening #1196 (if that can be done - otherwise I can make a new PR) seems like the next step.

So this doesn't happen again if we want to subclass Fields, which I want to do for this issue, I think going back to saving [(name, vocab), ...], i.e. old style vocabs, instead of saving fields should come next. I can do that.

What do you think @vince62s ?

[vince62s]

I think we will do as follow:

1. write a note on gitter + open an issue to recommend training on v0.7 until the next release.
2. you reopen Move batching and field logic from inputter to dsets #1196 with the 2 quick fixes for _feature_tokenize / make_img
   And we'll see what happens.
   are you guys ok ?

[bpopeters]

The saving/loading in 0.7.0 lets us refactor and subclass Fields as necessary and only worry about loading the vocab back into the right place in the field.

But this comes at a cost. In 0.7.0, the fields implementation can be changed at will because the options for fields are quite restricted and it is possible to reconstruct what fields to use from the vocab objects alone. Saving the fields allows an one to use an arbitrary structure of fields with a custom tokenizer, multiple fields for column of data, and so on. And that structure of fields can then be reloaded. Whereas it may not be possible to recover such a structure from a list of [(name, vocab), ...].

On the other hand, breaking checkpoint models is a serious problem. It's frustrating that fields do stuff that's important both at preprocessing time (tokenization) and training time (numericalization), since only a subset of these are actually relevant for either of these.

[flauted]

I understand your point. The solution needs more information and extensibility than [(name, vocab), ...]. The way I see it, it's not really that different from the relationship between a state_dict and a model class. The key problem is how do you initialize the model when you just have the weights. OpenNMT's solution is to save off all the model_opts. Could you just do that for fields? Save off all the opts so you can reconstruct the fields, and then load in the vocabs. That could pretty easily be backwards-compatible (prior to 14 days ago) too. Check for the opts in the checkpoint. If they're there, do the fields construction and inject the vocabs. If they're absent, do what the current code does for old-style vocabs.