chore: allow protobuf 3.20.3 requirement #22759

jose-turintech · 2023-04-14T06:42:50Z

Allow latest bugfix release for protobuf (3.20.3)

What does this PR do?

Change in requirements for python library so it allows the use of latest bugfix release for protobuf (3.20.3) instead of restricting it to the upper bound limit for this dependency to 3.20.2 (<=3.20.2).

Allow latest bugfix release for protobuf (3.20.3)

HuggingFaceDocBuilderDev · 2023-04-14T07:00:44Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

update auto-generated dependency table

amyeroberts · 2023-04-14T13:24:01Z

cc @ydshieh

ydshieh · 2023-04-14T14:20:53Z

Hi @jose-turintech Thank you for this PR ❤️ .

Unfortunately, as you can see in the CI results, the changes cause some errors

FAILED tests/models/albert/test_tokenization_albert.py::AlbertTokenizationTest::test_pickle_subword_regularization_tokenizer
FAILED tests/models/bert_generation/test_tokenization_bert_generation.py::BertGenerationTokenizationTest::test_pickle_subword_regularization_tokenizer

(Fatal Python error: Segmentation fault)

So we are not able to merge this PR so far. There might be some way to fix these 2 issues, but I am not sure. Let me know if you want to dive into this.

adriangonz · 2023-05-04T16:23:21Z

Hey @jose-turintech ,

Thanks for submitting this PR! The latest tensorflow==2.12 release depends on protobuf >= 3.20.3, so this would unblock installing the latest tensorflow alongside transformers.

After setting up this PR's environment, I just ran this locally and those tests seem to pass. Would it be possible to trigger a re-run @ydshieh? Alternatively, would it be possible to get extra logs on the CI failures?

jose-turintech · 2023-05-08T14:35:30Z

Hey @jose-turintech ,

Thanks for submitting this PR! The latest tensorflow==2.12 release depends on protobuf >= 3.20.3, so this would unblock installing the latest tensorflow alongside transformers.

After setting up this PR's environment, I just ran this locally and those tests seem to pass. Would it be possible to trigger a re-run @ydshieh? Alternatively, would it be possible to get extra logs on the CI failures?

Hello @adriangonz, just updated my branch with latest changes on origin in order to test if the PR check would retrigger. It seems so; so i guess we'll see if the PR passes all check within some minutes.

Thanks for your comment.

ydshieh · 2023-05-09T12:18:43Z

As you can see in the latest run, the 2 failed tests are still there.

The error (provided at the end below) is some processes crashed, and there is no more informative log being given by the pytest.

When I run the two failed tests individually on my local env., they pass.

However, since the latest release of tensorflow-probaility broke everything in the CI since we don't support TensorFlow 2.12 yet and it needs that version, we will take a more deep look to see if we can unblock this situation.

=================================== FAILURES ===================================
______ tests/models/bert_generation/test_tokenization_bert_generation.py _______
[gw0] linux -- Python 3.8.12 /home/circleci/.pyenv/versions/3.8.12/bin/python
worker 'gw0' crashed while running 'tests/models/bert_generation/test_tokenization_bert_generation.py::BertGenerationTokenizationTest::test_pickle_subword_regularization_tokenizer'
_______________ tests/models/albert/test_tokenization_albert.py ________________
[gw6] linux -- Python 3.8.12 /home/circleci/.pyenv/versions/3.8.12/bin/python
worker 'gw6' crashed while running 'tests/models/albert/test_tokenization_albert.py::AlbertTokenizationTest::test_pickle_subword_regularization_tokenizer'

jose-turintech · 2023-05-10T07:27:20Z

@ydshieh i've merged origin into this branch once again to retrigger CI checks just to see if test passed after the downtime of some huggingface transformers yesterday. Tests pass now after your modifications :) .

Only difference with main is the tensorflow-probaility>0.20 restriction is not applied in this CI build.

Thanks for taking the time to take a look into the issue.

ydshieh · 2023-05-10T10:38:39Z

@jose-turintech I accidentally pushed the experimental changes to this PR branch, I am really sorry. The CI is green as I removed some 3rd packages (tensorflow for example), which it should be kept.

I am still looking how to resolve the issue. There is a big problem (at least the desired environment when running inside CircleCI runner). I will keep you update soon.

ydshieh · 2023-05-10T10:39:39Z

In the meantime, let us convert this PR into a draft mode, so it won't be merged by accident. Thank you for your comprehension.

ydshieh · 2023-05-10T14:46:45Z

Issue

(for the hack to fix, see the next reply)

This PR want to use protobuf==3.20.3 so we can use tensorflow==2.12. However, some tests like test_pickle_subword_regularization_tokenizer fails with this environment. The following describes the issue.

First, setup the env. to have tensorflow-cpu==2.12 with potobuf==3.20.3 but without torch installed.
Use a sentencepiece tokenizer with enable_sampling=True
run the code with pytest --> crash (core dump)
- (run with a script --> no crash)
- (run the code with pytest where torch is also there --> no crash)

Here are 2 code snippets to reproduce (and not): run in python 3.8 will more likely to reproduce

create tests/test_foo.py and run python -m pytest -v tests/test_foo.py --> crash

from transformers import AlbertTokenizer
def test_foo():

    fn = "tests/fixtures/spiece.model"
    text = "This is a test for subword regularization."

    # `encode` works with `False`
    sp_model_kwargs = {"enable_sampling": False}
    tokenizer = AlbertTokenizer(fn, sp_model_kwargs=sp_model_kwargs)

    # test 1 (usage in `transformers`)
    # _ = tokenizer.tokenize(text)

    # test 2 (direct use in sentencepiece)
    pieces = tokenizer.sp_model.encode(text, out_type=str)

    # `encode` fails with `True` if torch is not installed and tf==2.12
    sp_model_kwargs = {"enable_sampling": True}
    tokenizer = AlbertTokenizer(fn, sp_model_kwargs=sp_model_kwargs)

    # test 1 (usage in `transformers`)
    # _ = tokenizer.tokenize(text)

    # This gives `Segmentation fault (core dumped)` under the above mentioned conditions
    # test 2 (direct use in sentencepiece)
    pieces = tokenizer.sp_model.encode(text, out_type=str)
    print(pieces)

create foo.py and run python foo.py -> no crash

from transformers import AlbertTokenizer

fn = "tests/fixtures/spiece.model"
text = "This is a test for subword regularization."

# `encode` works with `False`
sp_model_kwargs = {"enable_sampling": False}
tokenizer = AlbertTokenizer(fn, sp_model_kwargs=sp_model_kwargs)

# test 1 (usage in `transformers`)
# _ = tokenizer.tokenize(text)

# test 2 (direct use in sentencepiece)
pieces = tokenizer.sp_model.encode(text, out_type=str)

# `encode` works with `True` if torch is not installed and tf==2.12
sp_model_kwargs = {"enable_sampling": True}
tokenizer = AlbertTokenizer(fn, sp_model_kwargs=sp_model_kwargs)

# test 1 (usage in `transformers`)
# _ = tokenizer.tokenize(text)

# This works
# test 2 (direct use in sentencepiece)
pieces = tokenizer.sp_model.encode(text, out_type=str)
print(pieces)

ydshieh · 2023-05-10T14:51:54Z

(Hacky) Fix

The relevant failing tests are:

test_subword_regularization_tokenizer
test_pickle_subword_regularization_tokenizer

As mentioned above, those failing tests only happen when running with pytest. Furthermore, those test don't actually need protobuf in order to run. However, in the TF CI job, protobuf is a dependency from TensorFlow.

It turns out that running those 2 problematic tests in a subprocess will avoid the crash. It's unclear what actually happens though.

This PR modify those 2 tests to be run in a subprocess, so we can have protobuf==3.20.3 along with tensroflow==2.12.

ydshieh · 2023-05-10T14:57:37Z

The TF job is successful with the last fix, see this job run

https://app.circleci.com/pipelines/github/huggingface/transformers/64174/workflows/688a1174-8a6f-4599-9479-f51bbc2aacdb/jobs/793536/artifacts

The other jobs should be fine (we will see in 30 minutes) as they already pass before.

ydshieh · 2023-05-10T14:58:23Z

tests/test_tokenization_common.py

Run 2 tests in a subprocess to avoid problematic core dump

amyeroberts

Thanks for fixing and all the work getting the tests to work, including all of the details you provided, @ydshieh!

I just some comments on the assets and a question about _test_pickle_subword_regularization_tokenizer implementation

tests/test_tokenization_common.py

Co-authored-by: amyeroberts <[email protected]>

sgugger

Thanks a ton for taking the time to debug this!

ydshieh

Thank you again @jose-turintech for this PR, which allows to use TF 2.12!

jose-turintech · 2023-05-11T07:35:25Z

Thank you again @jose-turintech for this PR, which allows to use TF 2.12!

Thank you very much for taking the time to fix the issues, you did all the work; really appreciate it.

* chore: allow protobuf 3.20.3 Allow latest bugfix release for protobuf (3.20.3) * chore: update auto-generated dependency table update auto-generated dependency table * run in subprocess * Apply suggestions from code review Co-authored-by: amyeroberts <[email protected]> * Apply suggestions --------- Co-authored-by: ydshieh <[email protected]> Co-authored-by: Yih-Dar <[email protected]> Co-authored-by: amyeroberts <[email protected]>

chore: allow protobuf 3.20.3

62a09b5

Allow latest bugfix release for protobuf (3.20.3)

chore: update auto-generated dependency table

8cff738

update auto-generated dependency table

Merge branch 'huggingface:main' into patch-1

bfa6434

ydshieh self-assigned this May 9, 2023

ydshieh marked this pull request as draft May 10, 2023 10:38

run in subprocess

666b908

ydshieh force-pushed the patch-1 branch from 84b0fae to 666b908 Compare May 10, 2023 14:38

ydshieh marked this pull request as ready for review May 10, 2023 14:57

ydshieh reviewed May 10, 2023

View reviewed changes

tests/test_tokenization_common.py

Copy link

Collaborator

ydshieh May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run 2 tests in a subprocess to avoid problematic core dump

ydshieh requested review from sgugger and amyeroberts May 10, 2023 15:01

amyeroberts approved these changes May 10, 2023

View reviewed changes

tests/test_tokenization_common.py Outdated Show resolved Hide resolved

tests/test_tokenization_common.py Outdated Show resolved Hide resolved

tests/test_tokenization_common.py Outdated Show resolved Hide resolved

ydshieh and others added 2 commits May 10, 2023 18:30

Apply suggestions from code review

c5423e6

Co-authored-by: amyeroberts <[email protected]>

Apply suggestions

e643fe7

sgugger approved these changes May 10, 2023

View reviewed changes

ydshieh approved these changes May 10, 2023

View reviewed changes

ydshieh merged commit 0c65fb7 into huggingface:main May 10, 2023

ydshieh mentioned this pull request May 12, 2023

Use cu118 with cudnn >= 8.6 in docker file #23339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: allow protobuf 3.20.3 requirement #22759

chore: allow protobuf 3.20.3 requirement #22759

jose-turintech commented Apr 14, 2023

HuggingFaceDocBuilderDev commented Apr 14, 2023

amyeroberts commented Apr 14, 2023

ydshieh commented Apr 14, 2023

adriangonz commented May 4, 2023

jose-turintech commented May 8, 2023 •

edited

Loading

ydshieh commented May 9, 2023

jose-turintech commented May 10, 2023

ydshieh commented May 10, 2023

ydshieh commented May 10, 2023

ydshieh commented May 10, 2023 •

edited

Loading

ydshieh commented May 10, 2023

ydshieh commented May 10, 2023

ydshieh May 10, 2023

amyeroberts left a comment

sgugger left a comment

ydshieh left a comment

jose-turintech commented May 11, 2023

chore: allow protobuf 3.20.3 requirement #22759

chore: allow protobuf 3.20.3 requirement #22759

Conversation

jose-turintech commented Apr 14, 2023

What does this PR do?

HuggingFaceDocBuilderDev commented Apr 14, 2023

amyeroberts commented Apr 14, 2023

ydshieh commented Apr 14, 2023

adriangonz commented May 4, 2023

jose-turintech commented May 8, 2023 • edited Loading

ydshieh commented May 9, 2023

jose-turintech commented May 10, 2023

ydshieh commented May 10, 2023

ydshieh commented May 10, 2023

ydshieh commented May 10, 2023 • edited Loading

Issue

ydshieh commented May 10, 2023

(Hacky) Fix

ydshieh commented May 10, 2023

ydshieh May 10, 2023

Choose a reason for hiding this comment

amyeroberts left a comment

Choose a reason for hiding this comment

sgugger left a comment

Choose a reason for hiding this comment

ydshieh left a comment

Choose a reason for hiding this comment

jose-turintech commented May 11, 2023

jose-turintech commented May 8, 2023 •

edited

Loading

ydshieh commented May 10, 2023 •

edited

Loading