15 Adding CoreferenceProcessor #41
Conversation
…_coreference_processor
|
Please review the latest commits. |
|
Merge conflict for |
| @@ -83,12 +83,6 @@ jobs: | |||
| run: | | |||
| pip install --use-feature=2020-resolver --progress-bar off .[test] | |||
|
|
|||
| - name: Install Forte-wrappers-spacy | |||
There was a problem hiding this comment.
I think we shouldn't remove the installation directly from the workflow, let's actually use the workflow matrix set to run different dependencies as different runs.
Here is an example: https://github.com/asyml/forte-wrappers/blob/main/.github/workflows/main.yml#L84
There was a problem hiding this comment.
"use the workflow matrix set to run different dependencies as different runs."
Do you mean in the future we will have different versions of forte-wrapper, and you want to define a matrix to test them all?
There was a problem hiding this comment.
- No, not different versions of
forte-wrapper. I show you theforte-wrapperrepository workflow as an example of how to set up a workflow matrix. - In this repo, ForteHealth, we can set up a similar matrix, since not all processors in this repo can be installed and run at the same time, they can be tested individually. Note that you could use the
extra_installmethod insetup.pyto specify different sets of dependencies.
You may wonder how we make them work together in the future, that's another issue (we have solutions such as RemoteProcessor or Docker images). Right now we can focus on making them each testable.
There was a problem hiding this comment.
I still have a question: @Piyush13y seems to want to set forte.spacy as an extra_install. If so, forte.spacy and neurocoref will be in the same extra_install list, example:
setuptools.setup(
....
install_requires=[
"forte~=0.2.0",
],
extras_require={
.....
"coreference": [
"forte.spacy", # TODO: version
"cython>=0.25",
"neuralcoref @ git+https://git@github.com/huggingface/neuralcoref.git@4.0.0#egg=neuralcoref",
],
},
include_package_data=True,
python_requires=">=3.6",
)
I think pip won't make sure that forte.spacy is install ahead of neuralcoref.
There was a problem hiding this comment.
@Piyush13y seems to want to set forte.spacy as an extra_install.
Emmm... It's a little bit confusing because now forte.spacy is not even in the setup.py in master branch: https://github.com/asyml/ForteHealth/blob/master/setup.py
There was a problem hiding this comment.
Installing a spacy version compatible with neuralcoref causes the failure of SpacyProcessor. Since SpacyProcessor is needed in unit test, this seems to be a dead end before we can manage to run the modules separately.
There was a problem hiding this comment.
now we come back to my initial suggestion, use the matrix set up to run the module separately
There was a problem hiding this comment.
Bad news. A spacy version compatible with neuralcoref is not compatible with my RTX 3060.
I checked the issues in neuralcoref regarding the spacy version problem, almost everyone is suggesting building neuralcoref from source.
There was a problem hiding this comment.
The latest release of neuralcoref is in 2019. Maybe it is worth considering changing a better package.
There was a problem hiding this comment.
huh, actually for now we are simply looking for a coreference module that works. Maybe we can directly add this function to AllenNLPProcessor, or other packages you find easy
| # It is annoying that if we install neuralcoref and spacy at the same | ||
| # time, neuralcoref will throw "Cython failed" during building. | ||
| # Therefore, we must install neuralcoref after spacy is installed. | ||
| # git+https://git@github.com/huggingface/neuralcoref.git@4.0.0#egg=neuralcoref |
There was a problem hiding this comment.
We can deal with this through setup.py using extra requires.
| # properly. | ||
| # Therefore, we must install neuralcoref after cython and spacy | ||
| # are installed. | ||
| p = subprocess.call( |
There was a problem hiding this comment.
Can we use more standard way in setup.py
This PR fixes #15.
Description of changes
Added
CoreferenceProcessorand its unit test. Updatedsetup.py,requirements.txt, and ci workflow.CoreferenceProcessor: a wrapper forNeuralCoref. It translates the original output toCoreferenceGroupandMedicalEntityMentionand puts them into data pack.setup.pyandrequirements.txt: addedcython, andpytest, which are dependencies ofneuralcoref.fortex.spacy. Sinceneuralcorefneeds to be built from source. See the reason in NeuralCoref Installation Issue.Possible influences of this PR.
cythonandfortex.spacy.Test Conducted
Tested on new unit test.
NeuralCoref Installation Issue
Installing
neuralcoreffrom PyPI causes segmentation fault, since the binary file is built withspacy2.1, while thespacyused byforte.spacyis 2.3. We need to build from source to use customizedspacyversion.Building from source needs
spacyandcythonbeing already installed. Puttingneuralcoreftogether withspacyandcythoninrequirements.txtandsetup.pycan cause "Cython failed" exception, since building binary files comes ahead of the installation ofcythonandspacy. Therefore we need to delay the installation ofneuralcoref.But I don't know how to do this in an elegant way.
requirements.txtseems not to support this kind of two stage installation.setup.pyseems to support, but I don't know how to. So I added a new stage in CI after the installation ofrequirements.txtandfortex.spacy, treatingneuralcorefas an optional dependency likefortex.spacy.