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

Added konclude_test.py #231

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Added konclude_test.py #231

wants to merge 18 commits into from

Conversation

mpomarlan
Copy link
Collaborator

@mpomarlan mpomarlan commented May 31, 2022

(Work in progress)

Part of addressing #219

After the konclude_test.py script is accepted, the evaluation.yml file needs to be changed, and the docker configuration file too.

What else should be changed to update the CI?

@mpomarlan
Copy link
Collaborator Author

I've also updated the evaluation.yml file.

@sasjonge do you know how to write Docker files so that Konclude is also available?

Another couple of things to fix:

a) format. Konclude supports only OWL/XML and OWL functional syntax, not the RDF/XML that SOMA is written in. This needs an intermediate conversion step.
b) imports. These can be URIs to some web address and Konclude will find them just fine, however the same as above applies about the format. I would suggest here that we host DUL, and that we host an OWL functional syntax version of it. IOLite will be removed as an import if it hasn't been already so that's not important.

@sasjonge
Copy link
Collaborator

I can have a look. I saw that konclude already has an existing docker image:

https://hub.docker.com/r/konclude/konclude

Maybe we can use that, or atleast their Dockerfile to create our own image.

@sasjonge sasjonge marked this pull request as draft June 28, 2022 15:02
@sasjonge
Copy link
Collaborator

sasjonge commented Jul 5, 2022

I tested using the konclude container and it seems to generally work:

https://github.com/sasjonge/soma/blob/cf97673dd1122b616906d0f05331018e0a7cccc4/.github/workflows/evaluation.yml#L79

But if fails on this step, because a action is missing:

https://github.com/sasjonge/soma/blob/cf97673dd1122b616906d0f05331018e0a7cccc4/.github/workflows/evaluation.yml#L99

Did you forget to push it?

@mpomarlan
Copy link
Collaborator Author

Not so much forgot, as asking for assistance :) As far as I understand, the Konclude exec should be placed at the appropriate place by some kind of container setup. Don"t know how to do that.

@mpomarlan
Copy link
Collaborator Author

Actually wait, now looking at your previous message I don't understand what the problem is. Line 99 seems to just call Konclude, and if that is already there then where is the problem?

@sasjonge
Copy link
Collaborator

sasjonge commented Jul 5, 2022

@mpomarlan
Copy link
Collaborator Author

So the the problem is this:

2022-07-05T16:01:22.3350599Z ## Classes equivalent to http://www.w3.org/2002/07/owl#Nothing
2022-07-05T16:01:22.3350791Z
2022-07-05T16:01:22.3351106Z * -
2022-07-05T16:01:22.3351223Z
2022-07-05T16:01:22.3351228Z

? I will have a look on my side.

@sasjonge
Copy link
Collaborator

sasjonge commented Jul 5, 2022

The other outputs are always:

No issues detected.

For "## Classes equivalent to http://www.w3.org/2002/07/owl#Nothing" it is:

* -

@sasjonge
Copy link
Collaborator

sasjonge commented Jul 5, 2022

Can I merge my changes to the konclude-ci branch to extends this PR/Draft?
#247

@mpomarlan
Copy link
Collaborator Author

Pushed a commit that should fix this. The output will now be no issues detected if no classes are Nothing, and if there are unsatisfiable classes these will appear in a list.

NOTE: Konclude cannot test emptiness of properties. I may have mentioned this before, but just to make sure we're all reminded of this, here it is again. This shouldn't affect us too much as the typical way for a property to be empty is for either its domain or range to be empty. As long as we don't use intersections when specifying domains/ranges, we should be fine. If we use named concepts, or unions of named concepts, to specify domains/ranges, then the emptiness of the property would be caught by emptiness of the domain/range concepts.

Added docker run call to konclude
@sasjonge
Copy link
Collaborator

sasjonge commented Jul 5, 2022

I merged my code into this branch wirh this PR #247

@mpomarlan
Copy link
Collaborator Author

Good. If the ci now works, this is ready for review.

@mrnolte
Copy link
Collaborator

mrnolte commented Jul 7, 2022

NOTE: Konclude cannot test emptiness of properties. I may have mentioned this before, but just to make sure we're all reminded of this, here it is again. This shouldn't affect us too much as the typical way for a property to be empty is for either its domain or range to be empty. As long as we don't use intersections when specifying domains/ranges, we should be fine. If we use named concepts, or unions of named concepts, to specify domains/ranges, then the emptiness of the property would be caught by emptiness of the domain/range concepts.

Actually, it can via OWL link (I checked using my Server). Another option might be be to check for every ObjectProperty r whether r . some owl:Thing is empty, as that is only empty if either owl:Thing is empty (in which case something has gone terribly wrong already) or r is empty. @mpomarlan, is that possible using the Command Line?

Edit: The latter is not possible with OWL Link unfortunately - Konclude crashes 😅

@mpomarlan
Copy link
Collaborator Author

"Yes". As long as I have a list of object properties r, then one thing to do is to create a "query owl file" containing concepts of the form "r some Thing" for each r, then do a classification.

The one thing I'm not sure how to do right now is to get that list of properties. One option would be to check the list of declarations in the SOMA ugly file, assuming that all used object properties are also declared.

@mrnolte
Copy link
Collaborator

mrnolte commented Jul 8, 2022

The one thing I'm not sure how to do right now is to get that list of properties. One option would be to check the list of declarations in the SOMA ugly file, assuming that all used object properties are also declared.

It would not be a problem to generate such a list with the OWL API.

@mrnolte mrnolte marked this pull request as ready for review July 14, 2022 12:13
@sasjonge
Copy link
Collaborator

The one thing I'm not sure how to do right now is to get that list of properties. One option would be to check the list of declarations in the SOMA ugly file, assuming that all used object properties are also declared.

It would not be a problem to generate such a list with the OWL API.

Is this necessary to approve this PR?

@mpomarlan
Copy link
Collaborator Author

"Your" call -- for some value of "your".

As I said, what this means is the code as-is right now will not detect impossible properties unless they have impossible domains. This is maybe sufficient given that most of the domains we define are not intersections. On the other hand, we may have intersection domains, indeed the new versions of some of the GCIs were rewritten as such.

@mrnolte
Copy link
Collaborator

mrnolte commented Jul 26, 2022

If you tell me where to put such a list of properties, I will gladly export it so that you can then read it :)

@mpomarlan
Copy link
Collaborator Author

Would the same folder housing the merged SOMA be ok? (I know I can access that one for Konclude input :) )

@sasjonge
Copy link
Collaborator

I would rather recommend to have the path as an passed argument, so that we don't have to change the file if we change the locations in the pipeline later.

@sasjonge
Copy link
Collaborator

Yes, that looks okay.

@mrnolte
Copy link
Collaborator

mrnolte commented Jul 27, 2022

I now implemented exporting the properties. Just add a runtime argument propertyListPath to the java-ci stuff, e.g., as follows:

spring-boot:run "-Dspring-boot.run.arguments=--versionInfo=current --propertyListPath=<path X>"

This will create 2 files for each of the "main" ontologies (with that I mean the minimal set of ontologies, so that all ontologies from within the owl directory are imported - currently, these are Allen, Home and Elan. Interesting path problem to identify those btw). For example, <path X>/SOMA-HOME_objectProperties and <path X>/SOMA-HOME_dataProperties

@sasjonge
Copy link
Collaborator

sasjonge commented Aug 2, 2022

@mrnolte I have a problem using the flag. I get a ¨Could not create dir" error in CI and also locally: https://github.com/sasjonge/soma/runs/7629898757?check_suite_focus=true#step:5:376

Also would it be possible to make the propertyExporter optional, so that the other CI steps don't need to execute it (and get the argument)?

@mrnolte
Copy link
Collaborator

mrnolte commented Aug 2, 2022

@sasjonge this should have fixed it, can you check? Also, if propertyListPath is not given, the CI will now skip it.
Maybe a dumb question, but do we really have to call the java CI twice? Is there no option to generate the list in the first run and use it in the second?

@mpomarlan
Copy link
Collaborator Author

Updated the script. It will now optionally take an argument that gives a path to a file containing a list of object properties, each with its full IRI, one property per line.

The current line in the evaluation workflow reads:

python ./scripts/konclude_test.py -k ./.github/actions/Konclude -s ./build/SOMA-HOME.owl -o ./owl/konclude.html ./build/konclude.output

it should be

python ./scripts/konclude_test.py -p -k ./.github/actions/Konclude -s ./build/SOMA-HOME.owl -o ./owl/konclude.html ./build/konclude.output

Also please check if the paths to the Konclude binary (the -k arg) and the merged soma (-s) are correct.

@sasjonge
Copy link
Collaborator

@mpomarlan I tried to change it in the CI, but if you now want to use the konclude binary from inside of your script it will make the setup more difficult, because I can't just use the konclude docker file. I'm wondering if we could change the system call in your script to a docker run call?

https://github.com/sasjonge/soma/blob/621354d12cd88cb4ec6a11e675e4e026932dd95f/scripts/konclude_test.py#L65

@mpomarlan
Copy link
Collaborator Author

I don't know how to do that, but I suppose it should be fine. Can you implement it?

@sasjonge
Copy link
Collaborator

I tried to call the docker image from the python script on my fork (still a bit hacky with the pathes):

https://github.com/sasjonge/soma/blob/3d78aa40077784f0d8da824ebf997dfe2a099749/scripts/konclude_test.py#L66

This did run through:

https://github.com/sasjonge/soma/actions/runs/3337441052/jobs/5523785507

I printed out the SOMA_QUERY.owl in that CI. Should there only be ObjectSomeValuesFrom axioms in that file? Also the runtime is 0ms.

@mpomarlan
Copy link
Collaborator Author

The 0ms likely comes from this:

File '/data/SOMA-HOME.owl' not found.

When called, the Konclude binary needs to have access to both the SOMA_QUERY and SOMA_HOME.

@mpomarlan
Copy link
Collaborator Author

About SOMA_QUERY, the structure seems ok (I didn't notice Konclude complaining about parsing errors) and the content is intentional: when querying for properties, I only care about concepts of the form hasProperty some Thing (if hasProperty is equivalent to Bottom, then its domain is Nothing).

More interesting is that some properties have IRIs containing something else than SOMA or DUL, e.g. http://www.ease-crc.org/ont/SOMA-OBJ.owl#hasChildLink.

This is coming from the property list file I imagine.

@sasjonge
Copy link
Collaborator

About SOMA_QUERY, the structure seems ok (I didn't notice Konclude complaining about parsing errors) and the content is intentional: when querying for properties, I only care about concepts of the form hasProperty some Thing (if hasProperty is equivalent to Bottom, then its domain is Nothing).

More interesting is that some properties have IRIs containing something else than SOMA or DUL, e.g. http://www.ease-crc.org/ont/SOMA-OBJ.owl#hasChildLink.

This is coming from the property list file I imagine.

@mrnolte Is that with this different IRI's a problem with the script?

@sasjonge
Copy link
Collaborator

@mpomarlan
I get an parsing error:

XML parsing error at 1:1: 'Start tag expected.'.

My newest run is this:

https://github.com/sasjonge/soma/actions/runs/3338066719/jobs/5525234865

@mrnolte
Copy link
Collaborator

mrnolte commented Oct 27, 2022

It is a problem that @daniel86 explained to me: Someone who worked on SOMA-OBJ did not configure protege correctly and created all kinds wrong iris. His old script renamed these. I wondered whether old neems use the SOMA-OBJ IRI and did not change my script to also rename those IRIs, but that should be no problem after my vacation.

@mpomarlan
Copy link
Collaborator Author

The xml parsing error is not important as this is not an xml format, but the OWL functional syntax parsing error is. I think this is because the property list file has property IRIs with <> around the names whereas I assumed they would be without. If this is it then the fix is simple I'll have it done in a minute.

@mpomarlan
Copy link
Collaborator Author

Should be done now.

@mpomarlan
Copy link
Collaborator Author

Seems like a command line arg is wrong now, SOMA_HOM_objectProperties not found.

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.

3 participants