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

Update TextModels to work with Flux @0.12 and Zygote #20

Merged
merged 14 commits into from
Jul 11, 2021

Conversation

AdarshKumar712
Copy link
Contributor

@AdarshKumar712 AdarshKumar712 commented Apr 29, 2021

This PR updates the current version of the models available with TextModels.jl to work with [email protected] and Zygote. Will fix #1 and #19

Models updated so far:

  • CRF
  • NER
  • POS
  • Sentiment.jl (no changes)
  • averagePerceptronTagger.jl (no changes)

Also updated Project.toml

Work to be done:

  • Update ULMFit model
  • Update Tests
  • Update Docs (Need Suggestions on this!)

PS. Currently I have commented out the section of code belonging to ULMFit Model. Will uncomment it once its updated.

Please let me know if there are some mistakes.

cc @aviks

Update: Updated the tests and checked as well. For ULMFiT model, I am unable to verify the trainable part as the model is running very slow on my system. Rest of the models are working fine.
Update -2 : Documentation for some models, not available earlier is also moved from TextAnalysis.jl

fix errors in training

Correction in code for Text Classifier

Remove gpu erro
@sambitdash
Copy link

sambitdash commented May 1, 2021

I was working on fixing this. But didn't realise @AdarshKumar712 was working on it. So I have submitted all that I have done as part of PR #21.

In #21, I have made some changes to ULMFit, CRF, NER and POS. Some test cases in NER, POS are broken and have been marked as well. I feel there may be a retraining needed for NER and POS as the model is changed in Flux for the LSTM. The precomputed weights are not good enough. The retrainable parameter logic in ULMFit is probably broken. That may not be needed as well. One can provide the @functor macro the relevant params to set the right retrainable parameters.

@AdarshKumar712 feel free to use any code from #21 if you find it of use.

@AdarshKumar712
Copy link
Contributor Author

I will surely look into the changes you made in your PR. Thanks a lot @sambitdash!

@AdarshKumar712
Copy link
Contributor Author

Currently, CI build is giving a dependency error while precompiling because of CorpusLoaders.jl (used for ULMFit module). I have created an issue JuliaText/CorpusLoaders.jl#43 regarding the same. Once the issue resolves, hopefully CI build will work properly. And accordingly, I will also update this PR.

@aviks
Copy link
Member

aviks commented Jun 25, 2021

So @AdarshKumar712 is this ready to merge? There is a test failure in nightly, but I think that is an upstream issue.

@AdarshKumar712
Copy link
Contributor Author

Yes, this is ready for now. If there will be any issues, that can be accommodated in future. On a side note, the ULMFiT model is a bit way too slow. So wasn't able to completely check for the Pretraining and fine-tuning. But rest works fine.

@aviks aviks changed the title [WIP] Update TextModels to work with Flux @0.12 and Zygote Update TextModels to work with Flux @0.12 and Zygote Jun 25, 2021
src/ULMFiT/custom_layers.jl Outdated Show resolved Hide resolved
src/ULMFiT/custom_layers.jl Outdated Show resolved Hide resolved
src/ULMFiT/fine_tune_lm.jl Outdated Show resolved Hide resolved
@tejasvaidhyadev
Copy link
Member

looks good to me

@AdarshKumar712
Copy link
Contributor Author

Updated as per the suggestions. Hopefully, now it's ready to merge.
cc @aviks @tejasvaidhyadev

Copy link
Member

@tejasvaidhyadev tejasvaidhyadev left a comment

Choose a reason for hiding this comment

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

Looks good to me

@tejasvaidhyadev
Copy link
Member

@aviks
should we merge it?

@aviks
Copy link
Member

aviks commented Jul 11, 2021

Ok, here goes...

@aviks aviks merged commit e0c77f5 into JuliaText:master Jul 11, 2021
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 this pull request may close these issues.

Update TextModels for supporting flux-zygote
4 participants