Any23 295: Implement ability to use librdfa #104

JulioCCBUcuenca · 2018-07-19T03:20:49Z

Integration of librdfa on Any23 through the Rio API of RDF4J. For that, I have created a new parser.

In order to try out the PR, you should:

Install librdfa
Compile Any23

Signed-off-by: Julio Caguano

…d xml:lang is used to identify in xml files.

JulioCCBUcuenca · 2018-08-06T17:22:39Z

@lewismc just as a remainder, this will be the PR that I will be using in the last stage of GSoC.

lewismc

@JulioCCBUcuenca this is looking excellent as a first pass. Please see my comments.

lewismc · 2018-08-06T23:09:00Z

api/src/main/resources/default-configuration.properties

 # Allows to decide which RDFa Extractor to enable.
 # If 'on' will be activated the programmatic RDFa 1.1 Extractor
 # (org.deri.any23.extractor.rdfa.RDFa11Extractor) otherwise will be
 # registered the RDFa 1.0 legacy one (org.deri.any23.extractor.rdfa.RDFaExtractor).


There should be no mention of org.deri.any23 in this file. It should be org.apache.any23, can you please correct this. Thanks

lewismc · 2018-08-06T23:09:14Z

api/src/main/resources/default-configuration.properties


+# Allows to enable Librdfa Extractor.
+# If 'on' will override the extractors with the programmatic option,
+# RDFa 1.1 Extractor (org.deri.any23.extractor.rdfa.RDFa11Extractor) and 


Replace deri with apache

lewismc · 2018-08-06T23:09:41Z

core/pom.xml


+    <!-- BEGIN: Librdfa-RDF4J, loading from repository -->
+    <dependency>
+        <groupId>${project.groupId}</groupId>


Please use 2 space indents the same as the rest of the file.

lewismc · 2018-08-06T23:10:16Z

core/pom.xml

+    <dependency>
+        <groupId>${project.groupId}</groupId>
+        <artifactId>apache-any23-librdfa</artifactId>
+        <version>1.0.0</version>


Ensure that the versioning is consistent with the rest of the codebase e.g. 2.3-SNAPSHOT or whatever it is currently.

So, should I include librdfa-rdf4j as part of the build life-cycle?

lewismc · 2018-08-06T23:12:26Z

core/pom.xml

+  <repositories>
+    <repository>
+      <id>librdfa-rdf4j</id>
+      <url>https://raw.github.com/JulioCCBUcuenca/librdfa-java/repository/</url>


This is a bit hacky... what is meant to reside at this URL?

Since we haven't published the librdfa-rdf4j parser in maven central, I added the jars there. I added that because if while compiling librdfa is not installed, the build will fail. In that URL lives the jars of librdfa-rdf4j.

Currently, librdfa-rdf4j is a standalone module that is not part of any23. Thus, at compiling time, any23 doesn't know about librdfa-rdf4j. If we integrate this with any, we need to deal with the requirement that librdfa-rdfa needs librdfa.

librdfa-rdf4j should definitely be a separate module of Any23, please make sure that it is and that the require JAR dependencies are located at build and compile/runtime such.

lewismc · 2018-08-06T23:23:50Z

librdfa-rdf4j/src/main/c/main.java

+    caller.setCallback(callback);
+
+    caller.parse();
+    //rdfa.set_rdfa_parser(caller);


If not in use, please remove.

lewismc · 2018-08-06T23:24:40Z

librdfa-rdf4j/src/main/c/main.java

+
+  @Override
+  public void default_graph(String subject, String predicate, String object, int object_type, String datatype, String language) {
+    System.out.println("default_graph(...)");


Using System calls is never particularly safe... are all of these calls necessary?

lewismc · 2018-08-06T23:24:51Z

librdfa-rdf4j/src/main/c/rdfa.i

@@ -0,0 +1,40 @@
+%module(directors="1") rdfa


Missing license header.

lewismc · 2018-08-06T23:26:19Z

librdfa-rdf4j/src/main/resources/META-INF/services/org.eclipse.rdf4j.rio.RDFParserFactory

@@ -0,0 +1 @@
+org.apache.any23.rdf.rdfa.LibrdfaRDFaParserFactory


License header if possible.

lewismc · 2018-08-06T23:27:25Z

test-resources/src/test/resources/html/rdfa/basic.html

  limitations under the License.
 -->
-<html xml:lang="en" xmlns="http://www.w3.org/1999/xhtml">
+<html lang="en" xml:lang="en" xmlns="http://www.w3.org/1999/xhtml">


Seems like a strange addition. Can you explain?

The lang tag is used in HTML files, and the language of XHTML files is provided using xml:lang. So, since this is a HTML page, it needs to use the lang tag. Current XML parsers can use both lang or xml:lang, but since librdfa uses an old library for parsing XML it generates an error since it cannot identify the language.

Thats fine thanks for the explanation.

JulioCCBUcuenca · 2018-08-07T15:28:25Z

Thanks @lewismc for the feedback. I will work on all your suggestions.

lewismc · 2018-08-08T16:09:41Z

@JulioCCBUcuenca Please address the issue of making librdfa-rdf4j a module within Any23 then push your changes. At that stage I will most likely approve the PR and we can test more thoroughly. Thanks.

HansBrende · 2018-08-08T16:49:48Z

Current XML parsers can use both lang or xml:lang, but since librdfa uses an old library for parsing XML it generates an error since it cannot identify the language.

That seems worrisome to me... considering then, that most html pages will break the librdfa parser.

@lewismc should we not test more thoroughly before merging to master? Maybe a separate branch instead?

Also, I would think that all of our current rdfa parsing tests should be duplicated for the librdfa parser to ensure that it is at least as stable as our current rdfa parser. TBH, I'd be in favor of adding this into version 2.4 rather than 2.3 so we have more time to thoroughly test the module.

lewismc · 2018-08-08T17:29:20Z

@lewismc should we not test more thoroughly before merging to master? Maybe a separate branch instead?

Yes absolutely. We should also definitely extend the integration tests as well such that we can make more confident comparisons regarding runtime execution.

lewismc · 2018-08-08T17:29:47Z

TBH, I'd be in favor of adding this into version 2.4 rather than 2.3 so we have more time to thoroughly test the module.

That is fine with me...

JulioCCBUcuenca · 2018-08-08T19:51:58Z

Thanks for your comments @lewismc. Will do it! 😄

@HansBrende librdfa supports RDFa 1.0 and 1.1, so I made sure that librdfa extractor works on all tests that Semargl extractor runs for both RDFa 1.0 and 1.1; additionally, the librdfa extractor is tested on the base suitcase (AbstractRDFaExtractorTestCase ).

I agree that librdfa functionalities should go to other release. I am mostly concerned for the requirement that librdfa-rdf4j needs, which is that librdfa (the C library) needs to be installed. Remember, that not only the libraries that librdfa uses are old but the librdfa project itself has not being active in a while.

If we are moving to other branch, could someone create a new branch, so I can point the PR to the new branch.

I will be glad to keep contributing after GSoC. Honestly, I'd like the idea of becoming a commiter, so I have to keep working to earn it. 😃

lewismc · 2018-08-08T20:54:34Z

Remember, that not only the libraries that librdfa uses are old but the librdfa project itself has not being active in a while.

Yes but it is compliant with the RDFa test suite which is exactly what we want. It can be assumed to be somewhat complete...

If we are moving to other branch, could someone create a new branch, so I can point the PR to the new branch.

@HansBrende can you please create a new branch called ANY23-295, I do not have access to a terminal right now. Thank you in advance,

I will be glad to keep contributing after GSoC. Honestly, I'd like the idea of becoming a commiter, so I have to keep working to earn it.

👍

HansBrende · 2018-08-08T20:57:10Z

@JulioCCBUcuenca Great to hear it passes the semargl test suites! However, AbstractRDFaExtractorTestCase contains only a fraction of the tests we run on RDFa. You should also test against the RDFaExtractorTest and RDFa11ExtractorTest classes.

JulioCCBUcuenca · 2018-08-08T21:06:28Z

You should also test against the RDFaExtractorTest and RDFa11ExtractorTest classes.

@HansBrende it is testing those main tests as well. It is testing the tests of Semargl RDFa 1.0 (RDFaExtractorTest), Semargl RDFa 1.1 (RDFa11ExtractorTest), and base suitcase AbstractRDFaExtractorTestCase.

HansBrende · 2018-08-08T21:08:42Z

@JulioCCBUcuenca Alright, sounds great then!

HansBrende · 2018-08-08T21:15:21Z

@lewismc I attempted to create a new branch using:

git checkout -b ANY23-295
git push canonical ANY23-295

to which git responded with:

To https://git-wip-us.apache.org/repos/asf/any23.git
 * [new branch]        ANY23-295 -> ANY23-295

However, it isn't showing up under "Branches". Not sure why. It's showing up under my own "Branches" when I pushed to origin.

HansBrende · 2018-08-08T21:22:00Z

It is showing up here: https://git-wip-us.apache.org/repos/asf?p=any23.git

But not here: https://github.com/apache/any23/branches

HansBrende · 2018-08-08T21:29:32Z

Ok, apparently due to some quirkiness of the way mirroring works, new branches in git-wip will not show up on github until an actual change is made to the branch.

HansBrende · 2018-08-08T21:40:00Z

Ok, I added some whitespace to a package-info.java file, should be showing up now.

JulioCCBUcuenca · 2018-08-08T21:43:02Z

I changed the base branch. Thanks so much @HansBrende

…3-295

lewismc · 2019-09-11T16:08:24Z

librdfa-rdf4j/pom.xml

+  <parent>
+    <groupId>org.apache.any23</groupId>
+    <artifactId>apache-any23</artifactId>
+    <version>2.3-SNAPSHOT</version>


@JulioCCBUcuenca can you please update this to 2.4-SNAPSHOT so it is synchronized with master branch. Thank you

HansBrende · 2019-09-11T23:35:32Z

@lewismc do we have any benchmarks on this version vs our current rdfa parser?

lewismc · 2019-09-12T01:03:04Z

@HansBrende

https://cwiki.apache.org/confluence/display/ANY23/Librdfa-rdf4j+documentation#Librdfa-rdf4jdocumentation-Performance

The current implementation does not look particularly favorable for librdfa

HansBrende · 2019-09-13T01:46:23Z

@lewismc My first thought is: if the performance of this module is not as good as that of our current implementation, then in its current form, what is the added value?

My second thought is: the benchmarks do not test the Any23 Extractor wrappers around these rdf4j parsers, only the underlying parsers themselves. However, in Any23's BaseRDFExtractor, due to a lot of bugs in the semargl html parser, we had to preprocess the input stream using jsoup before passing "clean html" into the underlying parser. I am curious as to whether or not the librdfa parser would have any of those same html parsing bugs. If not, if I can take the preprocessing logic out of BaseRDFExtractor and move it to the semargl parser specifically, and if the librdfa parser can still pass the entire test suite without using the jsoup-preprocessed stream, then there would be a much better case for including it (as its performance would then likely eclipse our current rdfa performance without the preprocessing overhead).

HansBrende · 2019-09-13T02:22:37Z

I created an issue for this. We should resolve that issue first and then make sure the librdfa test suite still passes before adding the librdfa module.

HansBrende · 2019-09-15T01:41:22Z

@lewismc I've resolved that issue, moving the semargl-specific bugfixes into the semargl extractors.

@JulioCCBUcuenca a couple recommendations for this branch whenever you get a chance:

(1) Synchronize this branch with master
(2) Rerun test suite, making sure all tests still pass
(3) If possible, benchmark the Extractor wrappers (not just underlying rdf4j parsers). Doing this may give the librdfa extractor a performance edge, as the semargl parser requires the html stream to be preprocessed and transformed into strictly conforming XML.

lewismc · 2019-09-18T20:10:11Z

@HansBrende

My first thought is: if the performance of this module is not as good as that of our current implementation, then in its current form, what is the added value?

The project was started and completed as a research effort to see what results we could come up with. Thank you so much for the additional investigation on your part... I agree with your statement. I'll see if I can find some time to update the PR. Thanks

JulioCCBUcuenca added 13 commits June 28, 2018 22:03

Add extractors for librdfa.

a9f37b2

Signed-off-by: Julio Caguano

Merge branch 'master' of https://github.com/apache/any23 into ANY23-295

eec861c

Merge branch 'master' of https://github.com/apache/any23 into ANY23-295

ccf33ea

add librdfa extractor

6408269

Merge branch 'master' of https://github.com/apache/any23 into ANY23-295

2fe500f

ignore basic test

68f0d80

Make libdrfa configurable.

bd70dfc

solve integration test. Librdfa is loaded with SPI.

a271a21

Add test suite

5dbc86c

add last version of librdfa-rdf4j

c4b5dcc

Add lang tag. lang tag is used to identify language in HTML pages, an…

85e0c7e

…d xml:lang is used to identify in xml files.

Merge branch 'master' of https://github.com/apache/any23 into ANY23-295

b0a21ff

add librdfa-rdf4j as a separate module of any23

3ad4fc6

lewismc suggested changes Aug 6, 2018

View reviewed changes

JulioCCBUcuenca added 2 commits August 7, 2018 18:41

Add header files, use any23 version, and correct typo.

a7969e2

Merge branch 'master' of https://github.com/apache/any23 into ANY23-295

9527f38

JulioCCBUcuenca changed the base branch from master to ANY23-295 August 8, 2018 21:41

JulioCCBUcuenca added 2 commits August 8, 2018 18:01

Add librdfa-rdf4j

04f2adf

Merge branch 'ANY23-295' of https://github.com/apache/any23 into ANY2…

ac7b658

…3-295

lewismc approved these changes Sep 11, 2019

View reviewed changes

lewismc reviewed Sep 11, 2019

View reviewed changes

		@@ -0,0 +1 @@
		org.apache.any23.rdf.rdfa.LibrdfaRDFaParserFactory

Any23 295: Implement ability to use librdfa #104

Are you sure you want to change the base?

Any23 295: Implement ability to use librdfa #104

Uh oh!

Conversation

JulioCCBUcuenca commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JulioCCBUcuenca commented Aug 6, 2018

Uh oh!

lewismc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulioCCBUcuenca commented Aug 7, 2018

Uh oh!

lewismc commented Aug 8, 2018

Uh oh!

HansBrende commented Aug 8, 2018

Uh oh!

lewismc commented Aug 8, 2018

Uh oh!

lewismc commented Aug 8, 2018

Uh oh!

JulioCCBUcuenca commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lewismc commented Aug 8, 2018

Uh oh!

HansBrende commented Aug 8, 2018

Uh oh!

JulioCCBUcuenca commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HansBrende commented Aug 8, 2018

Uh oh!

HansBrende commented Aug 8, 2018

Uh oh!

HansBrende commented Aug 8, 2018

Uh oh!

HansBrende commented Aug 8, 2018

Uh oh!

HansBrende commented Aug 8, 2018

Uh oh!

JulioCCBUcuenca commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulioCCBUcuenca commented Jul 19, 2018 •

edited

Loading

JulioCCBUcuenca commented Aug 8, 2018 •

edited

Loading

JulioCCBUcuenca commented Aug 8, 2018 •

edited

Loading

JulioCCBUcuenca commented Aug 8, 2018 •

edited

Loading

HansBrende commented Sep 13, 2019 •

edited

Loading