gneissweb_classification

[ran-iwamoto]

Why are these changes needed?

This PR adds a transform for gneissweb classification using fasttext classifier.

Related issue number (if any).

issue #924

[ran-iwamoto]

Please check @shahrokhDaijavad @touma-I

[shahrokhDaijavad]

@touma-I and @ran-iwamoto I reviewed and modified the README file. It looks good now. I also tested both Python and Ray versions of notebook (after creating my own HF token and using that in the notebooks) successfully. CI/CD is failing above, because test-language-gneissweb_classification.yml is missing.

[Swanand-Kadhe]

One point for discussion: I was wondering if we want to call this transform as gneissweb classification transform. This is a more generic transform for fastText classification. We will use this transform with the fastText models used in GneissWeb to create GneissWeb recipe notebook. However, one can use this transform outside of GneissWeb. For instance, one can reproduce fastText classification step of DCLM dataset using this transform with the appropriate fastText model. It would be good to know what others think. Many thanks!

[shahrokhDaijavad]

One point for discussion: I was wondering if we want to call this transform as gneissweb classification transform. This is a more generic transform for fastText classification. We will use this transform with the fastText models used in GneissWeb to create GneissWeb recipe notebook. However, one can use this transform outside of GneissWeb. For instance, one can reproduce fastText classification step of DCLM dataset using this transform with the appropriate fastText model. It would be good to know what others think. Many thanks!

Thanks, @Swanand-Kadhe Your point is well-taken. However, we have discussed quite a bit about the naming of this transform in #924 and the name gneissweb_classification has been chosen, until we come up with a better name. The name fastText classification is too broad for what we have now.,

[shahrokhDaijavad]

See inline comment.

[Swanand-Kadhe]

Thanks, @Swanand-Kadhe Your point is well-taken. However, we have discussed quite a bit about the naming of this transform in #924 and the name gneissweb_classification has been chosen, until we come up with a better name. The name fastText classification is too broad for what we have now.,

Thanks a lot, @shahrokhDaijavad, for pointing me to the discussion in #924. I should have looked at it earlier. Sorry about that.

[shahrokhDaijavad]

Thank you, @ran-iwamoto, for making the latest changes. Let's wait for the other reviewers also to look before we approve and merge.

[cmadam]

Great implementation, however, I would suggest to use *_cli_param instead of *_key configuration keys in transform, as highlighted in the two comments below.

[shahrokhDaijavad]

@ran-iwamoto I pushed a few changes to your branch, to finalize the parameter names and configuration in README and the comment parts of the two notebooks.

[shahrokhDaijavad]

@BishwaBhatta This transform has been tested only with facebook/fasttext-language-identification model.bin
Whenever we have the models that were used in creating GneissWeb fasttext classification uploaded to HF or a similar public place, we should test it again.

[shahrokhDaijavad]

LGTM

change has been implemented. thanks