Upgrading Sweble and associated libs #155
Conversation
|
@diegoceccarelli this might be interesting for https://github.com/diegoceccarelli/json-wikipedia |
|
very interesting @tgalery |
|
@zesch @ferschke @daxenberger Would it help the integration if I set up Jenkins to allow building this PR to check its functionality? |
Probably, yes. Thank you |
|
fyi @diegoceccarelli we've built a model from this branch integrated to our json-wikipedia and we got a marginal bump in precision for English, but maybe this would be more significant for other languages. But maybe there are extra data structures in the new versions of the libs that could be explored too. Tests are passing besides a maven issue with some license warning, but do note that the test for the language configurator here makes an actual api call, this should be factored out before merge. |
|
This PR is related to #156 |
| @@ -22,7 +22,7 @@ | |||
| <parent> | |||
| <groupId>de.tudarmstadt.ukp.wikipedia</groupId> | |||
| <artifactId>de.tudarmstadt.ukp.wikipedia</artifactId> | |||
| <version>1.2.0-SNAPSHOT</version> | |||
| <version>1.2.1-SNAPSHOT</version> | |||
There was a problem hiding this comment.
Why would this PR change the version of the master branch?
There was a problem hiding this comment.
@reckart I use that to test a new dependency in a repo called json wikipedia, which we use for processing wikipedia dumps. I will revert to the original version
| String langCode = langCodes.get(0).name(); | ||
| return LanguageConfigGenerator.generateWikiConfig(langCode); | ||
| } else { | ||
| System.out.println(String.format("Cannot find language codes for %s", langName)); |
There was a problem hiding this comment.
Doesn't JWPL use a logging backend yet?
There was a problem hiding this comment.
This was used to test this function. My idea is to move the function that generates the language code to the Language enum class and if there's a logging backend there, I will use that. Otherwise I will remove the print statement
There was a problem hiding this comment.
Apache Commons logging should always be usable. If somebody wants to contribute more time, we could move to SLF4J, but that should be handled in a different issue.
|
Jenkins, can you test this please? |
|
Can one of the admins verify this patch? |
|
Jenkins, can you test this please? |
|
I had a quick look at the changes and they seem quite incremental to me. Nothing which immediately pops up like a major change or source of risk. Based on that review, I askedJenkins to do the build and it says the build is good with these changes. However, the patch changes the version number in some POMs. I think that should be reverted. So... how shall we proceed here? |
|
PR seems to be related to #152. |
|
Ah: either squash and change the commit message to reflect the issue number or rewrite the individual commit messages via an interactive rebase to reflect the issue number. |
|
@reckart I'm gonna do the fixes suggested above squash the commits and rebase on master, I might revert your master merge doesn't generate conflicts and squash everything with a commit message that addresses the issue. Probably will do that tomorrow. |
|
@tgalery what are the jitpack files for? Are they for a CI system? We have a Jenkins monitoring JWPL and by now it can also check PRs. |
|
I needed them to use github as a source for dependency resolution for json
wikipedia (again for purposes of testing). I'm happy to remove those
tomorrow too. The only thing that slightly worries me is the problems
pointed out here
#156 (comment) . But
maybe the api is being used in the wrong way. My use of this branch in json
wikipedia produced better better results and no problems AFAIK.
…On Sun, Apr 8, 2018 at 7:41 PM, Richard Eckart de Castilho < ***@***.***> wrote:
@tgalery <https://github.com/tgalery> what are the jitpack files for? Are
they for a CI system? We have a Jenkins monitoring JWPL and by now it can
also check PRs.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#155 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABwssBtUpkv5Kc7lFI6oABcylqqInpkFks5tmlnqgaJpZM4SYwcR>
.
|
|
@reckart I've delivered the changes dicussed and rebased against master to avoid conflicts. Let me know if there's anything else I can do. |
|
happy to sqash some commits if that makes everyone happier :-) |
|
Jenkins, can you test this please? |
|
@zesch @ferschke @daxenberger Jenkins says the patch is good. To me the changes look incremental rather than revolutionary. I'd tend to merge it. Does anybody else want to have a quick look at it and give thumbs up? |
|
Looks good, green light from my side. |
|
thanks @tgalery |
|
no problem @daxenberger . Would merging be the next step ? |
|
@tgalery actually, there is one more thing that we should have done before the merge, but I hope we can do it still ;) Could you please provide us with a contributor license agreement (http://dkpro.github.io/contributing/)? |
|
My bad, didn't see the email there. Just sent the signed pdf. Hope that's ok. |
|
All received, thanks! :) |
Connects to #152
In addition to the lib upgrade. I'm creating language specific configurations for the processed language. The test for that is a bit flaky as it makes an actual api call to wikipedia, but I inteded to add that just to show that it kind of works. More extensive testing for this would be done as part of the our use of https://github.com/idio/json-wikipedia/ .