support a compiled chain.model for improved performance

[erikerlandson]

This PR proposes to add a new compiled format for chain.model that pre-caches all the state transitions, as opposed to caching only the "begin".

I've been seeing generation 50 to 100x faster. Generation throughput increase depends on model size.

[coveralls]

Coverage remained the same at 100.0% when pulling 505c562 on erikerlandson:compiled-model into 15496b2 on jsvine:master.

[erikerlandson]

Note, this is basically similar to #102, but does it via a separate class.
Regarding memory, the memoized structures should be more compact, but I have had problems doing good memory profiling to prove it with hard numbers. I played with variations using np.array, which would be even better, however that would add a new project dependency, and I'm dubious it's worth the added performance

[erikerlandson]

An advantage of doing this via a separate class is that one can do:

model = CompiledText(model)

and so the original model can be garbage-collected to prevent redundant non-memoized structures from occupying a large amount of mem

[jsvine]

Thank you, @erikerlandson! This sounds both interesting and likely worth merging. I'll aim to take a closer look soon.

In the meantime, would you be willing to add a short description to the notes section here, right after the bullet point that begins "If you have accidentally read [...]"?

[erikerlandson]

@jsvine it is done 👍

[erikerlandson]

@jsvine have you had a chance to look at this?

[jsvine]

HI @erikerlandson, and thank you for the reminder! Now that I've had the chance to look at the code, I have a question about the implementation: Why not make CompiledText a subclass of Text? My concern is that many of the methods in CompiledText are simply Text methods. Ideally, when fixes or features come to the Text method, CompiledText would be able to automatically receive those, too, as a subclass (rather than having to manually keep CompiledText's features/fixes in sync with Text's. Or did you already consider subclassing and dismiss it as suboptimal?

[erikerlandson]

@jsvine, sorry I didn't see your comment 😦 My original reasoning about not making it a subclass is that not all Text methods are good fits for CompiledText. For example, actually training a CompiledText object directly is possible but I think better with traditional Text. Another operation I had in mind was combine, however that isn't actually defined on Text so it's not the problem I assumed.

If we can design it so that a sufficient number of methods are directly inherited, and the overloadings that need to be maintained are relatively stable, then I think it would be a good idea.

[erikerlandson]

@jsvine another possible strategy would be to define something like a TextBase that defines some common core API, and have both Text and CompiledText inherit from that.

[jsvine]

Thanks for the response, @erikerlandson! My thoughts below.

If we can design it so that a sufficient number of methods are directly inherited, and the overloadings that need to be maintained are relatively stable, then I think it would be a good idea.

Fantastic! I think you're right that some of the traditional Text methods might not make sense for CompiledText, and I think that's fine. For those, I think it's reasonable to raise a NotImplemented error, or something similar.

another possible strategy would be to define something like a TextBase that defines some common core API, and have both Text and CompiledText inherit from that.

Interesting! In the abstract, I can see how that might be useful. If you want to propose a rough (non-code) outline for how TextBase would differ from the current Text class, I'd certainly take a look. I'll also ruminate on this.

[erikerlandson]

@jsvine, it occurs to me that what is really being "compiled" is just the Chain object. Looking at the organization of Text, I am now thinking it would be most elegant to have a CompiledChain class, and modify a few of the core methods to do the right thing with either Chain or CompiledChain.

something like combine could just fail if it encounters a CompiledChain (and if someone feels motivated in the future, combine could augmented to support it, but I'm treating it as out of scope for this PR)

Doing it this way should also handle to_json and friends, and also test_sentence_output - if (like me) one also wants to avoid the cost of keeping the original text around, then that is already supported on Text.

[erikerlandson]

In fact, it's just the chain.model - I may try just adding a flag to encode whether model is compiled or not. Text might not need to be modified at all.

[jsvine]

That sounds like a smart, promising approach!

[erikerlandson]

I'll try to get to it soon, it's been a hectic couple of weeks :D

[erikerlandson]

@jsvine what do you think of this?

[jsvine]

Thanks, @erikerlandson! I'm impressed by how concise you were able to make the changes. I'll need a bit more time to think through this and review it more carefully, but on first glance it looks promising.

One question that initially comes to mind: Is it a better experience for users to have .compile(...) make in-place modifications, or should it return a new instance without modifying the original? (Off the top of my head, I believe that no other Model methods produce in-place changes; the class is, at least superficially, immutable.)

[erikerlandson]

@jsvine, I have no problem making it return a copy. I could support an in_place = False parameter, that would allow someone to compile it in-place if desired, but default to returning a new Text model

[erikerlandson]

@jsvine here's a tweak that makes non-destructive compile the default

[jsvine]

Many thanks @erikerlandson! These changes look great. Merged, and now available in v0.8.0. Cheers!