Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing tests with python 3.10 due to changed error message #957

Closed
h-vetinari opened this issue Mar 21, 2022 · 3 comments · Fixed by #962
Closed

Failing tests with python 3.10 due to changed error message #957

h-vetinari opened this issue Mar 21, 2022 · 3 comments · Fixed by #962

Comments

@h-vetinari
Copy link
Contributor

This is showing up in conda-forge/tokenizers-feedstock#40, almost certainly because the text of the error being raised changed with python 3.10:

____________________ TestCustomNormalizer.test_instantiate _____________________

self = <tests.bindings.test_normalizers.TestCustomNormalizer object at 0x7fd354094ee0>

    def test_instantiate(self):
        bad = Normalizer.custom(TestCustomNormalizer.BadCustomNormalizer())
        good_custom = TestCustomNormalizer.GoodCustomNormalizer()
        good = Normalizer.custom(good_custom)
    
        assert isinstance(bad, Normalizer)
        assert isinstance(good, Normalizer)
        with pytest.raises(Exception, match="TypeError: normalize()"):
>           bad.normalize_str("Hey there!")
E           Exception: TypeError: TestCustomNormalizer.BadCustomNormalizer.normalize() missing 1 required positional argument: 'wrong'

The good news is that these seem to be the only relevant errors:

=========================== short test summary info ============================
FAILED tests/bindings/test_normalizers.py::TestCustomNormalizer::test_instantiate
FAILED tests/bindings/test_pre_tokenizers.py::TestCustomPreTokenizer::test_instantiate
================== 2 failed, 132 passed, 1 skipped in 57.25s ===================

Well, aside from the fact that windows does not support forking, so this line should be guarded to not run on windows.

@Narsil
Copy link
Collaborator

Narsil commented Mar 21, 2022

Yes this is known, we just need to update the test to reflect that, instead of checking the full string we can just check the common part (owned by tokenizers)

@h-vetinari
Copy link
Contributor Author

Just noticing now that this is raising an Exception masquerading as a TypeError. Is there now way to raise the correct error type, or does that get lost in the pyo3-layer somewhere?

@Narsil
Copy link
Collaborator

Narsil commented Mar 22, 2022

I don't remember about this issue, I just remember that 3.10 and 3.9 different in the exact text message, but it doesn't matter too much since what we really want is to make sure that the error message contains the core information.

For the error type, I would need to look a bit more into it, definitely depends on what's happening if it's a rust panic or a handled rust error cast into python exception for instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants