spacy-experimental
spacy-experimental copied to clipboard
explosion
Coref Components
This is a continuation of https://github.com/explosion/spaCy/pull/7264, since we decided to add the coref components here first. It's still a work in progress.
There may still be a few rough edges in the code, but I believe at this point it's ready for review, and strange types / lack of clarity should be mostly resolved.
You can also test a fully working component with the project here. https://github.com/explosion/projects/pull/101
Tests are failing due to an error in thinc related to trying to unpack a dlpack twice. The failures are in the xp2torch function and affect components besides coref. I cannot reproduce this on a local machine with the same thinc/spaCy versions, so I think it may be GPU-specific. Still looking into the details.
Thanks to @adrianeboyd for pointing out this error is due to having an old Torch and new numpy, which aren't compatible. I have upgraded the Torch version used in the tests to 1.10.0, which fixed the issue locally.
I think it would be useful to have an end-to-end test including the scoring with a custom prefix. It's easy to miss a place where it's accidentally just using the defaults.
Minor in the scheme of things, but I think you can skip tests at the module level instead of having lots of repeated method-level skipifs:
https://docs.pytest.org/en/6.2.x/skipping.html#skip-all-test-functions-of-a-class-or-module
A more general global marker might be useful in the spacy test suite later.
I assume you're waiting on the spacy docs to decide how to add this to the README?
I assume you're waiting on the spacy docs to decide how to add this to the README?
Yes, it wasn't clear if details should go here or in the spaCy docs. Since it looks like we're using the spaCy docs I'll add enough info here to point to them.
If the note in the docs is OK, I think that all the concerns about this have been addressed, and this should be ready to be merged.
Yes, I think it makes sense to follow up on performance later.
To summarize the results of profiling, it looks like most of the time is in Torch layers, and there's nothing that can be easily optimized that would have significant impact. I'm not super familiar with Torch optimization so I may have missed something, but I didn't see anything like expensive loops that could be vectorized, for example.
OK, now that performance profiling is done for the time being I think this is ready to merge?
OK, there was a small issue with the optimizations on Windows, but now the tests have cleared (including project tests on Windows) and I was able to train a whole pipeline without issue, so this should be ready to merge, pending any further review.
I am going to see what happens if we add Windows to the CI, though if there are issues with other components that could be out of scope for this PR.
Well, it looks like the tests for this repo on Windows just work, so I guess we can add that too if we want.