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

Add test for textcat CNN issue #357

Merged
merged 6 commits into from Mar 3, 2023

Conversation

polm
Copy link
Contributor

@polm polm commented Dec 14, 2022

This is a test demonstrating the issue in explosion/spaCy#11968. A potential fix is being worked on in explosion/thinc#820.

In its current condition, the test just creates a pipeline with textcat and transformer, creates a minimal doc and calls nlp.initialize. As described in the spaCy issue, that fails with this error:

ValueError: Cannot get dimension 'nI' for model 'linear': value unset

This will be left in draft until the fix is clarified.

@polm
Copy link
Contributor Author

polm commented Dec 29, 2022

EDIT: This comment belongs on the Thinc PR, so I have posted it there as well.

I would like someone more familiar with the resizable layer to look at this, but I believe the current fix is correct. To clarify what's going on:

Before this PR, the layer is assumed to be initialized if nO is set. This causes an error in the TextCatCNN when used with transformers, because the number of classes (nO) is known (or at least temporarily assumed) when the textcat component is created, but the size of the transformer embedding (nI) is not known until later.

After this PR, the resizable layer is considered uninitialized if either nO or nI is unset. Because nO may already be set in that case, the new nO is set using force=True.

@polm polm marked this pull request as ready for review December 29, 2022 08:40
@polm
Copy link
Contributor Author

polm commented Dec 29, 2022

I made one small change, so close/reopening to trigger tests...

@polm polm closed this Dec 29, 2022
@polm polm reopened this Dec 29, 2022
requirements.txt Outdated Show resolved Hide resolved
@svlandeg svlandeg added the tests Missing or incorrect tests label Jan 5, 2023
@polm
Copy link
Contributor Author

polm commented Jan 6, 2023

@explosion-bot please test_slow_gpu --thinc-branch refs/pull/820/head

@explosion-bot
Copy link
Collaborator

🚨 Errors

Command test_slow_gpu --thinc-branch refs/pull/820/head is not enabled for this repo.

@polm
Copy link
Contributor Author

polm commented Jan 6, 2023

@explosion-bot please test_gpu --thinc-branch refs/pull/820/head

@explosion-bot
Copy link
Collaborator

explosion-bot commented Jan 6, 2023

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/spacy-transformers-gpu-test-suite/builds/21

@polm
Copy link
Contributor Author

polm commented Jan 6, 2023

With the branch change reverted, the non-buildkite builds are failing as expected.

The buildkite build is failing with an error that there's no such module as spacy_transformers.align - I'm not sure what would cause that, as that has nothing to do with this PR. It looks like these tests haven't been triggered in a PR for a few months so I'm not sure if something is off about the build or something.

@polm
Copy link
Contributor Author

polm commented Jan 10, 2023

@explosion-bot please test_gpu --thinc-branch refs/pull/820/head

@explosion-bot
Copy link
Collaborator

explosion-bot commented Jan 10, 2023

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/spacy-transformers-gpu-test-suite/builds/22

@polm
Copy link
Contributor Author

polm commented Jan 10, 2023

@explosion-bot please test_gpu --thinc-branch refs/pull/820/head

@explosion-bot
Copy link
Collaborator

explosion-bot commented Jan 10, 2023

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/spacy-transformers-gpu-test-suite/builds/23

@polm
Copy link
Contributor Author

polm commented Jan 10, 2023

Because this relies on a particular Thinc branch, the Azure tests are failing, but the Buildkite test using the proper Thinc branch has succeeded.

Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Apart from the small comment, this looks good. We'll need to release Thinc and bump its lower pin here so the CI goes green. Then we can get this merged and published.

@adrianeboyd
Copy link
Collaborator

It feels a little weird to change the spacy requirement for the package just because of a test, so maybe instead we can have the test restricted to spacy>=3.5.1? It's still a little odd, though.

@svlandeg
Copy link
Member

svlandeg commented Mar 3, 2023

Nice solution, Adriane! 🙏

@svlandeg svlandeg merged commit 9dac4e5 into explosion:master Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Missing or incorrect tests
Projects
None yet
4 participants