MOZI-AI / annotation-scheme

Human Gene annotation service backend
GNU General Public License v3.0
3 stars 4 forks source link

test failure #70

Closed linas closed 4 years ago

linas commented 4 years ago

OK, got build and install to work. Now make check fails:

$ make check
make  check-TESTS
make[1]: Entering directory '/home/ubuntu/src/annotation-scheme/build'
make[2]: Entering directory '/home/ubuntu/src/annotation-scheme/build'
FAIL: tests/parser-tests.scm
Makefile:779: recipe for target 'tests/util-tests.log' failed
make[2]: *** [tests/util-tests.log] Error 1

So

$ cat tests/util-tests.log
Backtrace:
           1 (primitive-load-path "tests/util-tests.scm")
           0 (primitive-load "tests/sample_dataset.scm")

ERROR: In procedure primitive-load:
In procedure open-file: No such file or directory: "tests/sample_dataset.scm"
linas commented 4 years ago

OK ... debugging ...

This suggests that there is either some file that is not checked into git that defines these functions, or that its some external guile module that isn't being imported ...?

linas commented 4 years ago

Also several tests fails with exceptions:

test-name: parse-request
location: /home/ubuntu/src/annotation-scheme/tests/main-tests.scm:21
source:
+ (test-equal
+   "parse-request"
+   5
+   (length
+     (parse-request (list "IGF1") "Dfaer" req)))
expected-value: 5
actual-value: #f
actual-error:
+ (wrong-type-arg
+   "map"
+   "Wrong type argument: ~S"
+   (#((("filters"
+        .
+        #((("value" . "smpdb reactome")
+           ("filter" . "pathway"))
+          (("value" . "True") ("filter" . "include_prot"))
+          (("value" . "False") ("filter" . "include_sm"))
+          (("value" . "0") ("filter" . "biogrid"))))
+       ("function_name" . "gene-pathway-annotation"))
+      (("filters"
+        .
+        #((("value"
+            .
+            "biological_process cellular_component molecular_function")
+           ("filter" . "namespace"))
+          (("value" . "0") ("filter" . "parents"))
+          (("value" . "True") ("filter" . "protein"))))
+       ("function_name" . "gene-go-annotation"))
+      (("filters"
+        .
+        #((("value" . "True") ("filter" . "coding"))
+          (("value" . "True") ("filter" . "noncoding"))))
+       ("function_name" . "include-rna"))
+      (("filters"
+        .
+        #((("value" . "Proteins")
+           ("filter" . "interaction"))))
+       ("function_name"
+        .
+        "biogrid-interaction-annotation"))))
+   ())
result: FAIL

I find it odd that it says "wrong-type-arg "map"` because one of the warning that gets printed is

WARNING: (annotation functions): `map' imported from both (rnrs base) and (srfi srfi-1)

which suggests ... the wrong map is being used. There is a work-around from this, in guile you can somehow say "from foo import blah as bar" to rename the imported symbol (map, in this case) to not collide with other symbols of the same name.

Habush commented 4 years ago

The file annotation/parser.scm uses a function called edge-info-pubid-set! but this is not defined anywhere......

These are imported from the (json) module which you can find here

Also several tests fails with exceptions:

@tanksha can you check this? may be the new addition of the rna annotation broke it ?

linas commented 4 years ago

Performing grep -r edge * and grep -r pubid * on the json module gives exactly zero hits. This is for version 3.3.0 from October 2019. Very clearly, it would seem that these functions were removed, and no backwards compat layer was provided. That's actually very unfortunate; you're going to have to go out to the old versions, figure out what these functions did, and the wheedle and plead with the maintainer to provide a backwards-compat layer. That really kind-of sucks. I'm so sorry. Ooooof. `

linas commented 4 years ago

Anyway, the unit tests now pass for me (after the assorted pull reqs), so maybe you don't actually need this function? I also ran gene-pathway-annotation gene-go-annotation and biogrid-interaction-annotation (with parents=0) and never hit this function ... maybe its just old code that isn't being called any more?

rekado commented 4 years ago

@linas this project unfortunately uses a fork of Guile JSON. These procedures don't seem to ever have been available in the upstream releases of Guile JSON.

linas commented 4 years ago

Huh? Which project?

rekado commented 4 years ago

I cannot reproduce the test failure when building with the fork of Guile JSON. The tests fails with upstream Guile JSON.

linas commented 4 years ago

what fork? what upstream? I have 6 or 8 forks going right now, one fork for each open pull req. After applying all of these, all unit tests now pass for me, If I recall, the test failures were really due to misconfigured makefile.am and configure.ac. One these are straightened out, everything works fine. I'm waiting on the pull reqs to get merged; I'll close this then.

rekado commented 4 years ago

I'm talking about "Guile JSON". This project (MOZI-AI/annotation-scheme) uses a fork of "Guile JSON", namely this one: https://github.com/Habush/guile-json

This is the upstream of Guile JSON: https://github.com/aconchillo/guile-json

These two have diverged.

(I was only able to even use the Habush fork after patching its Makefile to have compiled Guile modules installed in the conventional location.)

rekado commented 4 years ago

FWIW, I see only one problem in this project's Makefile.am --- the use of an undefined m4 macro. This is addressed in the first commit of this PR: https://github.com/MOZI-AI/annotation-scheme/pull/100

tanksha commented 4 years ago

There is no test failure now. @linas I think you used the Guile Json from https://github.com/Habush/guile-json which fixes the issue. closing this issue.

linas commented 4 years ago

I am using upstream guile-json.

@tanksha please re-open. DO NOT EVERT CLOSE A BUG REPORT AND SAY THAT THERE IS NO TEST FAILURE (expletive deleted) YOU DO NOT HAVE A LOGIN ON MY MACHINE, AND YOU HAVE NO WAY WHATSOEVER TO TEST TO SEE IF THERE IS A FAILURE OR NOT

This is extremely rude. Mike Duncan ask for my help because you guys have accumulated vast quantities of https://en.wikipedia.org/wiki/Technical_debt and for you to just close this and baldly tell me to my face something that is completely untrue, denying the bare, basic facts -- that's just crazy. Sorry to yell. But seriously, you guys have to address the fundamental problems that exist, and do so in a methodical, direct productive manner, instead of just claiming that everything is fine and nothing needs to be done.

tanksha commented 4 years ago

@linas sorry. I thought you were having the error dueto the json repo issue.

rekado commented 4 years ago

@linas

I am using upstream guile-json.

This is precisely the problem. You should not be using upstream Guile JSON, but the project- specific fork as mentioned in the Readme file.

I'm not a developer of this project and I ran into the same problem you had using the original Guile JSON. Upon realizing my mistake I built the fork of Guile JSON, noticed that it installs the compiled objects in the wrong location, patched it (in the Guix package definition, see PR), and then managed to run the tests just fine.

I find it rather rude to insist that the problem lies with other people when independent people (e.g. me) have verified that there is no problem with this code base (well, no problem with the test suite anyway). There's no need to be condescending to other developers.

linas commented 4 years ago

Um, Look, this is really incredibly frustrating. It took me about a week to build this project. To find and fix all of the kinks and whacks in this project, so that it merely BUILDS. Building the project should have taken half an hour, not a week. I am very frustrated that all of you guys are like "works for me" when obviously, you have all sots of environments set up that aren't out-of-the-box normal environments.

I was explicitly asked to help you guys figure out what all of the problems and issues are, and then to help you fix them, and then when I do this, the response is "lah-dee-dah it works for me, not a problem, close". Don't ask me for help, and then when I help, tell me that this help isn't needed.

Regarding guile-json, it is straight-forward, its not magic: https://www.google.com/search?q=guile+json and the very first link that pops up is this: https://github.com/aconchillo/guile-json If I then go to https://github.com/aconchillo/guile-json/pulls tehe are zero open pull requests, so it is safe to assume that whatever forks you are on about, they have been merged, or they have been rejected. The maintainer seems to be reasonable, so I assume that any/all outstanding forks have been merged. Now, I'm not a mind-reader, but you guys seem to think that I am, and so out of thin air, you came up with some other diff, that, let me cut and paste it from the other bug: here bug #93 -- I have no idea how you even found this other fork -- presumably, because it is well-known to you guys ... but there is no particular way for me to obtain this bit of information.

Which now begs the question -- why is guile-json forked? What happened to that fork? Why did the guile developer reject that fork? What steps did you take to remediate the situation? It's insane to create a fork, and then, when that fork is rejected BY THE MAINTAINER, to ignore the situation, and build yet more code that requires a rejected fork! Oh, and to keep all this as an internal-only, only-the-collaborators-know-about-it kind of thing.

Again: please take the time and effort to read every last word in this wikipedia article: https://en.wikipedia.org/wiki/Technical_debt -- and stop to think about it, and what it means, with respect to this project. You have clearly incurred significant technical debt -- you have a project that won't build, won't pass unit tests, depends on a secret fork of some code the maintainer of which has rejected, and then are in total denial that there is even a problem, telling me that everything just works fine, when that is obviously just not true. This is a classic software-developer behavior pattern, and it is never a good sign. it's a symptom of a development shop on the verge of total breakdown. I cannot single-handedly prevent that breakdown, or pull you away from the edge of total failure. You have to participate in this rescue process in some kind of reasonable, professional kind of way.

Habush commented 4 years ago

why is guile-json forked?

I forked guile-json to add extra records and fields to help me parse the atomese to json.

What happened to that fork? Why did the guile developer reject that fork? What steps did you take to remediate the situation?

I didn't send any PR upstream. The reason I didn't send any PRs is because this fork is specific to our use-case and doesn't help other users of the upstream repo.

Oh, and to keep all this as an internal-only, only-the-collaborators-know-about-it kind of thing.

It is not internal, it is publicly available on github [here] (https://github.com/Habush/guile-json) . I don't know how github's search engine works but apparently it doesn't show you the forks of the project and will only show you the main project.

I admit that I should have explicitly stated which fork the project used and that is a mistake on my part.

rekado commented 4 years ago

you have all sots of environments set up that aren't out-of-the-box normal environments

This is not correct. I started from scratch after packaging all dependencies (cogutils, atomspace, etc) for Guix a few days ago. I wrote the Guix environment definition file (see PR) based on what this project's readme file stated at the time, which includes a link to https://github.com/Habush/guile-json. (It didn't notice this at first, because it was hidden behind the text "Guile JSON".) And then I used that to create a suitable development environment.

In my opinion it is better to abandon the fork. As I expressed elsewhere, there is no good reason to build the graph while a JSON data structure is constructed, and Atomese is already in S-expr format, so parsing doesn't make much sense in a language where S-exprs are the native format. Instead the Atomese expression could be evaluated recursively, cutting out Guile JSON completely.

linas commented 4 years ago

OK.

I forked guile-json to add extra records

As a general rule of software development, you shouldn't do that. Imagine, for a moment, that instead you had forked glibc to add these records, or even forked the linux kernel, and then tried to get the rest of the world to use your forked version, instead of the maintainer's version. That simply would not work, and this example is a smaller-scale example of the same.

Instead, you would create a fork of glibc, or the kernel, if you were creating a feature that you were pretty certain that the glibc or kernel maintainers would accept and merge. Thus, the fork is temporary, until the maintainers accept/reject the merge. Users should never be asked to use the forked version. Exactly the same rules apply to guile-json, and to pretty much every other project, with a few extreme exceptions involving abandon-ware.

If I click through to https://github.com/Habush/guile-json I immediately see this, at the top of the screen: This branch is 15 commits ahead, 75 commits behind aconchillo:master. This means that the maintainer, aconchillo, has made 75 bug fixes, improvements, enhancements that are simply missing in the habush fork. As a user, one would typically want to use the version with all the fixes and enhancements (the aconchillo version) and not something much older and out-of-date and apparently unmaintained (which is why it is so far behind master).

If I look at the actual differences: https://github.com/aconchillo/guile-json/compare/master...Habush:master its .. well, its hard to make out what it does. Apparently, the master guile-json has no concept of nodes and edges, so this adds those concepts. It's layering some kind of graph/hypergraph interface onto json. So this is a new, "orthogonal" feature, and its usually better if orthogonal features are implemented into their own module, providing a distinct function.

Overall, I'm unclear on the new function. Atomese is about hypergraphs, and we call them "nodes" and "links" to maintain a distinction between "vertexes" and "edges" of ordinary graph theory. Otherwise, they are more-or-less the same thing: a node is a vertex, and edge is a link. Atomese is a graph-processing API.

It might be nice to have a graph-processing API on top of json. But it should be a distinct module.

It might be nice to have a simpler graph API for Atomese, but no one has sat down to do this.

It might be nice to have a json format for Atomese, but no one has sat down to define one.

It might be nice to have a javascript API to Atomese, but no one has sat down to do that ...

linas commented 4 years ago

FWIW, as of today, (28 Feb 2020) the unit tests still fail for me:

PASS: tests/parser-tests.scm
PASS: tests/util-tests.scm
PASS: tests/pathway-tests.scm
FAIL: tests/main-tests.scm
============================================================================
Testsuite summary for gene-annotation 0.1
============================================================================
# TOTAL: 17
# PASS:  13
# SKIP:  0
# XFAIL: 0
# FAIL:  4
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
============================================================================

The failing tests are:

test-name: parse-request
location: /home/ubuntu/src/annotation-scheme/tests/main-tests.scm:29
source:
+ (test-equal
+   "parse-request"
+   5
+   (length
+     (parse-request (list "IGF1") "Dfaer" req)))
expected-value: 5
actual-value: #f
actual-error:
+ (wrong-type-arg
+   "map"
+   "Wrong type argument: ~S"
result: FAIL

test-name: validate-functions
location: /home/ubuntu/src/annotation-scheme/tests/main-tests.scm:33
source:
+ (test-equal
+   "validate-functions"
+   1
+   (length
+     (parse-request (list "IGF1") "Dfaer" invalid-req)))
expected-value: 1
actual-value: #f
actual-error:
+ (wrong-type-arg
+   "map"
+   "Wrong type argument: ~S"
result: FAIL

test-name: annotate-genes
location: /home/ubuntu/src/annotation-scheme/tests/main-tests.scm:35
source:
+ (test-assert
+   "annotate-genes"
+   (string?
+     (annotate-genes (list "IGF1") "Dfaer" req)))
actual-value: #f
actual-error:
+ (wrong-type-arg
+   "map"
+   "Wrong type argument: ~S"
result: FAIL

test-name: protein-goterm
location: /home/ubuntu/src/annotation-scheme/tests/main-tests.scm:41
source:
+ (test-equal
+   "protein-goterm"
+   86
+   (length
+     (cog-outgoing-set
+       (find-proteins-goterm
+         (GeneNode "IGF1")
+         namespace
+         0))))
expected-value: 86
actual-value: 85
result: FAIL
Habush commented 4 years ago

@linas most of the test failures are due to the map function in srfi-1. That's odd. can it be a system specific issue?

Habush commented 4 years ago

Disregard my previous comment. I reproduced the test failures. Working on a fix

Habush commented 4 years ago

The reason for the test failures is that the upstream guile-json repo now converts JSON arrays to vectors instead of lists as it used to. I will send a PR to fix this issue.

rekado commented 4 years ago

Yes, that's why I let the Docker image and the guix.scm specify the use of the older Guile JSON that used lists.

Habush commented 4 years ago

@linas are the test still failing on your side after the latest PR?

linas commented 4 years ago

Looks like they all pass!

ubuntu@opencog-mozi:~/src/annotation-scheme/build$ make check
make  check-TESTS
make[1]: Entering directory '/home/ubuntu/src/annotation-scheme/build'
make[2]: Entering directory '/home/ubuntu/src/annotation-scheme/build'
PASS: tests/parser-tests.scm
PASS: tests/util-tests.scm
PASS: tests/pathway-tests.scm
PASS: tests/main-tests.scm
============================================================================
Testsuite summary for gene-annotation 0.1
============================================================================
# TOTAL: 17
# PASS:  17
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
make[2]: Leaving directory '/home/ubuntu/src/annotation-scheme/build'
make[1]: Leaving directory '/home/ubuntu/src/annotation-scheme/build'