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

Dimension inference in Coref #11089

Merged
merged 15 commits into from
Jul 12, 2022
Merged

Conversation

polm
Copy link
Contributor

@polm polm commented Jul 6, 2022

Description

This PR is for doing dimension inference in coref instead of requiring manually specifying input size. PyTorch based layers need to know their size when their __init__ function is called or serialization doesn't work.

The current status is that for the coref component this works now, but serialization still doesn't work. If this can be figured out the span predictor will still need similar changes.

Types of change

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

This follows the pattern used in the Biaffine Parser, which uses an init
function to get the size only after the tok2vec is available.

This works at first, but serialization fails with an error.
@polm polm added the feat / coref Feature: Coreference resolution label Jul 6, 2022
@polm
Copy link
Contributor Author

polm commented Jul 6, 2022

I believe this works now, but this should be run with the changes + tests in #11042 first - SpanPredictor didn't have any tests before the ones in that PR were added.

@polm
Copy link
Contributor Author

polm commented Jul 8, 2022

As a note, the failing tests here are just type issues, which should be resolved by #11087.

@polm
Copy link
Contributor Author

polm commented Jul 11, 2022

With the other PRs merged, I'll run the tests here before merging it in.

@polm
Copy link
Contributor Author

polm commented Jul 11, 2022

This PR added some variables that didn't have types, which caused mypy tests to fail.

Besides that a textcat test that shouldn't be affected by any of these PRs seems to be failing. I'll look into that more.

@polm
Copy link
Contributor Author

polm commented Jul 12, 2022

@explosion-bot please test_gpu

@explosion-bot
Copy link
Collaborator

explosion-bot commented Jul 12, 2022

🪁 Successfully triggered build on Buildkite

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

polm added 2 commits July 12, 2022 14:07
There's no guarantee about the order in which SpanGroup keys will come
out, so access them in sorted order when doing comparisons.
This was necessary when the tok2vec_size option was necessary.
@polm
Copy link
Contributor Author

polm commented Jul 12, 2022

@explosion-bot please test_gpu

@explosion-bot
Copy link
Collaborator

explosion-bot commented Jul 12, 2022

🪁 Successfully triggered build on Buildkite

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

@polm polm marked this pull request as ready for review July 12, 2022 06:53
@polm
Copy link
Contributor Author

polm commented Jul 12, 2022

OK, tests passed, so merging to get all changes in one place.

@polm polm merged commit 90973fa into explosion:feature/coref Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat / coref Feature: Coreference resolution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants