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

Updating third party tools to newer versions and removing MaltParser fixed version #2832

Merged
merged 10 commits into from Oct 13, 2021

Conversation

gavishpoddar
Copy link
Contributor

@gavishpoddar gavishpoddar commented Sep 30, 2021

Hello,

This micro pull request updates third party tools to newer versions (Stanford corenlp, postagger, parser) (#2826) and removes MaltParser fixed version. (#2827)

I am trying the fix the above-listed issues please let me know if changes are required.

Thanks

Copy link
Member

@tomaarsen tomaarsen left a comment

Regarding the changes to the travis file - I'm afraid we won't need those. And for the MaltParser, we're looking for a more involved fix that tackles the weird dynamic I've described in my other comment.

It's up to you whether you want to try and tackle those issues, or if you want to leave it. If you choose the former, then maybe it would be best to wait until #2820 is either closed or merged.

tools/travis/third-party.sh Outdated Show resolved Hide resolved
nltk/test/gluesemantics_malt_fixt.py Outdated Show resolved Hide resolved
@gavishpoddar
Copy link
Contributor Author

@gavishpoddar gavishpoddar commented Oct 7, 2021

Thanks, @tomaarsen, I see that is now merged and I am trying to update this PR.

And if there is anything I can contribute please let me know

Thanks

@tomaarsen
Copy link
Member

@tomaarsen tomaarsen commented Oct 7, 2021

Of course! Perhaps the links in https://github.com/nltk/nltk/blob/develop/tools/github_actions/third-party.sh that gather the third party applications can be updated. Beyond that, any problem from https://github.com/nltk/nltk/issues can be tackled.

@tomaarsen tomaarsen closed this Oct 7, 2021
@tomaarsen
Copy link
Member

@tomaarsen tomaarsen commented Oct 7, 2021

Apologies, I did not mean to close this.

@tomaarsen tomaarsen reopened this Oct 7, 2021
@gavishpoddar gavishpoddar requested a review from tomaarsen Oct 10, 2021
@tomaarsen
Copy link
Member

@tomaarsen tomaarsen commented Oct 13, 2021

I'm trying to do some testing, but it seems that GitHub is having some issues with their windows-latest continuous integration, which is making it all a bit more difficult. I'll get back to this later.

Copy link
Member

@tomaarsen tomaarsen left a comment

I've removed the parser_dirname parameter from a few test cases, where the path to the actual executable should be supplied via the MALT_PARSER environment variable. This would completely fix the issue #2826! I believe with your changes, this should pass just fine.
If it does, then I'll merge this, and reset the CI cache so that it indeed downloads the new versions of MaltParser and the CoreNLP tools for all future test runs.

@tomaarsen tomaarsen linked an issue Oct 13, 2021 that may be closed by this pull request
@tomaarsen tomaarsen linked an issue Oct 13, 2021 that may be closed by this pull request
@tomaarsen tomaarsen merged commit e7a1f31 into nltk:develop Oct 13, 2021
16 checks passed
@tomaarsen
Copy link
Member

@tomaarsen tomaarsen commented Oct 13, 2021

These changes look great. Thank you @gavishpoddar!

@gavishpoddar
Copy link
Contributor Author

@gavishpoddar gavishpoddar commented Oct 13, 2021

Thanks, @tomaarsen for helping me throughout the PR and the merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants