Break lifetime entanglement of TextExtract, TFIDF and Jieba

[awong-dev]

Modify KeywordExtract to take a Jieba instance during the keyword extraction call instead of during construction. Remove the Jieba reference and the lifetimes from struct TextRank as well as struct TFIDF.

This is desirable in situations where the extractor instance might outlive a single function call.

One specific case where this comes up is in binding from other languages where the Jieba instance may be refcounted and shared between API calls in a way that depends on the other languages's calling semantics. In these situations, it is hard to have TextRank and TFIDF be bound to the Jieba instance via Rust's understanding of scoped-based lifetimes.

Luckily, the KeywordExtract interface only uses the Jieba instance on execution of the extraction and arguably, the API should just take the Jieba instance in (or even possibly the resulting segments) to reduce coupling of the structs. In this PR, the Jieba instance was moved from construction down to the function where it was used.

Since this is an API breaking change, the PR also cleans up the API style per This brings the API more in line with Rust conventions per

https://rust-lang.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits

Specifically:

• TFIDF was renamed TfIdf to follow Rust's capitalization conventions for acronyms.
• KeywordExtract::extract_tags() was renamed to KeywordExtract::extract_keywords()
• A KeywordExtractConfig object was created holding common code from both TextRank and TfIdf.
• {TextExtract,TfIdf}::new() no longer encodes defaults and just the algorithm parameters as well as a KeywordExtractConfig object
• TextExtract and TfIdf both implement the Default trait and expose a default() method with the default configuration.
• API tests moved into doc tests further collocating code with test.

CAVEAT: In the new code, TextExtract defaults to NOT use hmm in the Jieba cutting. In the old code TextExtract had hmm on and TFIDF had hmm had it off. This inconsistency looks like an oversight.

fixes #99

[awong-dev]

This a more concrete example of what I was thinking in #99. I don't really love it...but wanted to throw it out there for consideration.

Having gotten this far, I wonder maybe one could try adding a

pub trait JiebaKeywordExtract {
    fn extract_tags(&self, &jieba: Jieba, sentence: &str, top_k: usize, allowed_pos: Vec<String>) -> Vec<Keyword>;
}

moving all of the impl over to two TFIDFNoJieba and TextExtractNoJieba structs w/o the Jieba instance. And then maintaining the existing API by turning TFIDF<'a> and TextExtract<'a> into thin facades that just contain the Jieba reference and an instance of one of the above.

The final result would be a 2 APIs, one where &Jieba is bound on new(), and one where it is bound on run...

[awong-dev]

@messense I think this is ready for review now if you have a sec. The CI failures seem to be due to incorrect access tokens in github...

[messense]

Thanks! I'll take a look next week.

[awong-dev]

Note there is a behavior change with TextExtract. I can undo it, but honestly the more I look at this, the more I wonder if it'd be worth it to break API compat and remove the older APIs.

[messense]

Note there is a behavior change with TextExtract. I can undo it, but honestly the more I look at this, the more I wonder if it'd be worth it to break API compat and remove the older APIs.

It's fine to introduce breaking changes, a semver bump isn't a big issue.

[awong-dev]

Note there is a behavior change with TextExtract. I can undo it, but honestly the more I look at this, the more I wonder if it'd be worth it to break API compat and remove the older APIs.

It's fine to introduce breaking changes, a semver bump isn't a big issue.

Oh! In that case, hold off on review for a bit. Let me just replace the Old APIs instead of introducing awkward names like JiebaKeywordExtract, etc.

And if we're going to do a semver bump, before publishing, let me also try to make the hmm replaceable (either by completing the other open PR or writing a new one).

Give me a week. Will ping again.

[awong-dev]

Okay, I think I have everything stable now.

The capi has NOT been updated to reflect the newer more flexible API. It just preserves the old semantics.

If we are okay continuing down this path, I think there are some follow-up PRs that would

1. Add a Default trait for Jieba and make Jieba::new() not assume any defaults.
2. Extract and Hmm struct out of Jieba and make cut take an Option<&Hmm> parameter instead of true/false
3. Update capi to reflect the new strcuture.

This would be a cohesive api restructuring in one semver bump.

[messense]

Please run cargo fmt to format code.

[awong-dev]

Please run cargo fmt to format code.

Done though it didn't marke the pub(cargo) thing...just the change in jieba_benchmark.rs

I also realized the PR title + summary were completely out of date so I rewrote them.

[messense]

I'd remove getters (not really useful) and use builder pattern for KeywordExtractConfig, like

let config = KeywordExtractConfig::builder()
    .add_stop_word("word")
    .use_hmm(true)
    // and other options
    .build();

or without a separate builder type:

let config = KeywordExtractConfig::default()
    .add_stop_word("word")
    .use_hmm(true)
    // and other options
    ;

[messense]

We can change this later, does not need to block merging this PR.

[awong-dev]

Added #101 to track.

[messense]

Thank you for your contribution, I've sent you an invitation to collaborate on this project.

[awong-dev]

Thank you!!

For the getters, filing an issue. I kinda wanna mess with completing that custom HMM extraction in the other open PR first, then attempt to bind these all into elixir to see what happens.

I worry that w/o the getters, it might be harder to query the configuration into other languages without having to duplicate (and synchronize) the state.