Addons: Implement Conditional Random Field

[seanpmorgan]

Per the RFC, we need to move CRF from contrib to addons:

This will involve moving the code and modifying test cases to run in TF2.

[a6802739]

@seanpmorgan, may I give some help on this?

[seanpmorgan]

Absolutely! Please let us know if you run into any issues. As noted in the other issue, one thing to note is that the RNN cells are unified under Keras RNN in TF2.

[a6802739]

@seanpmorgan, thanks for your help, I will ping you if I met some issues.

[a6802739]

@seanpmorgan, Sorry for late interaction. So what I should do now is move CRF from contrib to the directory addons/tensorflow_addons, and change the corresponding test in tensorflow, right?

[seanpmorgan]

Hmmm move the code and change the tests/code in addons to match the TF2 syntax would be how I would word that.

[a6802739]

@seanpmorgan, Thanks for you help, and could you please give me some PR reference that I could follow?

As my understanding, I should move the code to addons and add crf_test.py within adding/crf and guarantee the unit test style match the TF2 syntax, for example using test.TestCase and add py_test in BUILD file, right?

And how I should deal with the function defined in contrib/crf, should I use class to implement a wrapper or just move the code directly?

I'm appreciate for your kind comments.

[seanpmorgan]

Hi @a6802739

As my understanding, I should move the code to addons and add crf_test.py within adding/crf and guarantee the unit test style match the TF2 syntax, for example using test.TestCase and add py_test in BUILD file, right?

Yes that is correct.

And how I should deal with the function defined in contrib/crf, should I use class to implement a wrapper or just move the code directly?

This is where I'm confused. You'll need to move the code from contrib, but then modify it to use the unifed RNN class in TF2. This will come with some changes to the code so that it runs correctly. To be honest, this is not the most straightforward of all the issues we need help with. It might be easier to move something that already has an example PR we can point you to. For example moving an optimizer and then you can match how LazyAdam was moved.

[a6802739]

@seanpmorgan , Thanks for you kind help, I will have a try then. Could you please point me the example PR and the ongoing issue about optimizer.

[NLP-ZY]

How is it going on about this issue? I want to use tf.keras(2.0), but there is no CRF Layer, so that I can only use keras & keras_contrib.

[seanpmorgan]

Hi @NLP-ZY thanks for bumping this. CRF is a highly requested feature and we'll make sure it's implemented prior to a TF2 release.

Relevant issue: https://github.com/tensorflow/tensorflow/issues/26167

[Squadrick]

@seanpmorgan I'll work on this.

[howl-anderson]

If need help, I familiar with CRF and I can help with the coding and testing.

[seanpmorgan]

IMO this should go under tfa.text. Any objections to that?

[Squadrick]

Yup, I was thinking the same.

[NLP-ZY]

@seanpmorgan Thanks for your reply!

[Squadrick]

@howl-anderson Some of my tests are failing because they're off from the actual values. I have my code here. I'll take a look at it later today, but if you want you could take a crack at it.

[howl-anderson]

@Squadrick Sure

[seanpmorgan]

@invencode @soloice @nlp-zy We're having internal discussion about how we want to expose the CRF functionality in Addons. Would it be sufficient to only expose it as a Keras Layer, or do you have use cases were the lower level op calls would be used?

[NLP-ZY]

@seanpmorgan The CRF layer in keras is good enough for me in the moment.

[howl-anderson]

The CRF layer of the tf.addons I'm working on is basically ready, but the code still needs more testing to ensure correctness and robustness. Will be submitted in the next one or two weeks.

[seanpmorgan]

@howl-anderson Does your implementation use the code in #314 or is it standalone? Would you mind submitting a WIP PR so we can better plan?

[howl-anderson]

@seanpmorgan The current implementation of my CRF layer use functions from #314 . I will submit it as a WIP PR soon. I will keep you informed.

[howl-anderson]

@seanpmorgan For keep you informed, WIP PR #377 is for the CRF layer.

[napsternxg]

IMO this should go under tfa.text. Any objections to that?

Sorry to restart the discussion on this. Thanks for including CRF in TF 2.0 addons. However, it should be noted that CRF is a general methodology for any type of sequence tagging task where the prediction has to be made on each item of the sequence. This includes videos, audio, text, and even graph traversals.
I don't think putting it under text is the best approach.

[foxik]

Maybe adding it directly to tfa.layers? But personally I do not care if it is tfa.layers or tfa.text, given that span labeling of text seems to be one of the most common application of linear CRF.

[napsternxg]

I think tfa.layers is a better place. I don't have an issue either, just thought that in the earlier contrib module it was not under text, hence, this change might cause discovery bias towards users in non-text domain.