Implementing Bloom Annotator

[ian-cho]

Why are these changes needed?

Add an additional column to indicate whether the document is present in the pre-existing Bloom filter model. This is specifically intended for the GneissWeb release.

[shahrokhDaijavad]

@ian-cho I reviewed the README file and ran python local_python.py successfully on my laptop. Please add details to issue #981.
Also, as done by all transforms, please add a simple notebook for the Python run-time (and a link to it from the README file).

[ian-cho]

@shahrokhDaijavad @touma-I added notebook for python runtime and also put a link to it from README file https://github.com/ian-cho/data-prep-kit/blob/dev/transforms/universal/bloom/bloom_python.ipynb

[shahrokhDaijavad]

@ian-cho I haven't had time to test the notebook, but I just realized that you are using bf.bloom model. Is that open source and available to download, so we don't need to include it in the repo?

[ian-cho]

@shahrokhDaijavad thanks for pointing this out. Currently, I am not aware of any small, downloadable models okay for this demonstration. Thus, I trained a small model locally. In the future, we can replace this local model path with a Hugging Face model when it is ready later I guess.

[shahrokhDaijavad]

@shahrokhDaijavad thanks for pointing this out. Currently, I am not aware of any small, downloadable models okay for this demonstration. Thus, I trained a small model locally. In the future, we can replace this local model path with a Hugging Face model when it is ready later I guess.

Thank you, @ian-cho. The size of the model 240KB is quite small, but for us to include it statically in an IBM-owned open repo, we need permission from IBM. I will bring this up with Bhatta, who may be able to get permission for us.

[shahrokhDaijavad]

@BishwaBhatta As you may know, @ian-cho is using a small model that he has trained himself for this transform. This is fine for testing, but we cannot merge the code with that model included in the outer repo. So, if this small model will not be used at the end and an HF model will replace it, after successful testing, should we take it out before merging the code?

[ian-cho]

@shahrokhDaijavad Hi, thanks for asking.

• I tested the gneissweb.bloom (28GB) that Yohei trained, the current bloom transform code work with it. So, from my side, it is okay to remove that small model and move forward. But I am open to @BishwaBhatta's opinion.
• @BishwaBhatta asked if the code can process fineweb parquet which has more than 30 columns. Among others, the document column is called text not contents, I think we have been through this naming issue for HAP, so shall we stick to the name contents instead of text as in test1.parquet? Otherwise, I would like to upload new test1.parquet.
  Thank you!

[shahrokhDaijavad]

@shahrokhDaijavad Hi, thanks for asking.

* I tested the _gneissweb.bloom_ (28GB) that Yohei trained, the current bloom transform code work with it. So, from my side, it is okay to remove that small model and move forward. But I am open to @BishwaBhatta's opinion.

* @BishwaBhatta asked if the code can process fineweb parquet which has more than 30 columns. Among others, the document column is called _text_ not _contents_, I think we have been through this naming issue for HAP, so shall we stick to the name _contents_ instead of _text_ as in [test1.parquet](https://github.com/ian-cho/data-prep-kit/blob/dev/transforms/universal/bloom/test-data/input/test1.parquet)? Otherwise, I would like to upload new test1.parquet.
  Thank you!

Thank you, @ian-cho, for testing with the large model. For consistency with everything else in DPK, I think we should stick with the contents instead of text for the document column.

[ian-cho]

@shahrokhDaijavad ok, then can I comment out this line ? The transform does not need text or contents. By doing this, we can stick to contents naming and also users do not have to change the name of fineweb parquet.

[shahrokhDaijavad]

@shahrokhDaijavad ok, then can I comment out this line ? The transform does not need text or contents. By doing this, we can stick to contents naming and also users do not have to change the name of fineweb parquet.

@touma-I Do you see any problem with what @ian-cho is asking?

[shahrokhDaijavad]

@ian-cho Sorry that I did not test the notebook until today. I did a make venv and in my venv, tried python local_python and it ran successfully. I had deleted the local output folder first (it should not have been put in the repo, as it gets created after the run, so please delete it). After the run, the output folder was created and had the 2 metadata.json and test1.parquet files were there. However, if I run the notebook, I get the error below:

What am I missing? Can you please look into this? Thanks.

[ian-cho]

Hi, @shahrokhDaijavad, I have no ideas why it failed. Your and @touma-I 's help would be appreciated!

[shahrokhDaijavad]

@ian-cho After @touma-I fixed the workflow test failure, I saw that test-src was failing and I pushed the same fix that we did for using the new API and now everything is passing in CI/CD.
I think the only remaining issue is that we still have the small bf.bloom file included. If this is a meaningless model that is just for testing, it may be ok keeping it (like small meaningless parquet files we use for testing).

[ian-cho]

@touma-I @shahrokhDaijavad Thanks for your fix! Retaining bf.bloom would make the repo self-contained if it is fine to keep it. Also, I added two comments here and here for the user's convenience for filtering out FineWeb into GneissWeb if it is okay.

[shahrokhDaijavad]

Thank you, @ian-cho. Some gneissweb fasttext models were added to HuggngFace yesterday. Who is adding the gneissweb.bloom (28GB) that Yohei trained to HF?
I am approving this PR and pass it to @touma-I, before it is merged.

[shahrokhDaijavad]

LGTM.

[shahrokhDaijavad]

@ian-cho As I was going over the README file with @touma-I today, we felt that it was not clear enough for our audience in the open-source community why we are doing this. So, I made some changes in the README file. Can you please review my changes and see if you want to modify/add anything? Thanks.

[ian-cho]

Hi, @shahrokhDaijavad @touma-I , thanks. The added summary looks good to me.

[ian-cho]

@touma-I @shahrokhDaijavad @BishwaBhatta (@issei-ibm) I updated the model path in the scripts to ibm-granite/GneissWeb.bloom. However, during on-the-fly testing, I run into the OOM error due to the model's 28GB size. any ideas? Thanks a lot!

[shahrokhDaijavad]

Thank you for trying this out, @ian-cho. I am not surprised, because I would definitely get an OOM on my laptop that has 16GB memory. How much memory do you have on your laptop? When you said a couple of weeks ago (see above) that you tried it with the 28GB model that Yohei trained, how did you do it?
In any case, if we have some idea of the minimum memory needed to try this successfully when the 28GB file is downloaded on the fly, we should put that in the README file. Most community users though won't have access to machines with more than 16GB memory. Maybe you should put back the small bf.bloom so that local testing succeeds, but in the README, we point and link to the real model file on HF/IBM-granite.

[ian-cho]

@shahrokhDaijavad No, on my laptop, it was successful to load 28GB model with the on-the-fly test. When I push it to DPK, one test failed due to OOM error as follow:

See from 497th line.

[shahrokhDaijavad]

Great. Thank you @ian-cho. So, the OOM happens in the Ubuntu server that runs CI/CD tests. Please keep the code as is with the real model. If your laptop has more than 16GB of memory, please let me know, and I will add a sentence to the README file, stating that "using the large model file will require a machine with more than 16GB of memory".

[ian-cho]

@shahrokhDaijavad Hi, Thanks. My laptop has more than 16GB memory.

[shahrokhDaijavad]

@ian-cho Can you please review the changes I made to the README and to the notebook? Thanks.

[ian-cho]

Hi, @shahrokhDaijavad, thank you for making these changes. It looks good to me. Just two quick questions:

• Is laptop with 16GB sufficient to load 28GB bloom filter model? (Mine actually has 64GB memory)
• As for bloom_python.ipynb, only the OOM error remains, while everything else seems fine after the modification? Thanks a lot.

[shahrokhDaijavad]

@ian-cho I made a small change in the comment cell of the notebook that mentions pip install. With the new APIs, we don't need to pip install data-prep-toolkit anymore.
As for your questions, in my laptop that has only 16GB of memory, when I tried today, I got a Jupyter kernel crash when I ran the notebook, so 16GB is not enough.
For your second question, yes, I am fine with approving this PR (even with the CI/CD error), but I am not able to merge and Maroun has asked me not to ask other admins who can merge until he comes back next week. Is there an urgent need to merge this before next week?
I am able to create a pip install release on PyPi that includes the bloom transform, even if it is still in PR and not merged. Is there an urgent need for that?

[ian-cho]

@shahrokhDaijavad Thank you for the modification. In HuggingFace’s release of the GinessWeb Bloom filter, the link to DPK now points to the dev branch of my personal Git repository. It should be pointing to the DPK outer branch after the merge.

Understood, @ian-cho We will change the link after the merge.