opencog / pln

Probabilistic Logic Network (PLN) implemented on top of the Unified Rule Engine (URE). https://wiki.opencog.org/w/Probabilistic_logic_networks
Other
15 stars 19 forks source link

Undefined symbol when importing PLN Python bindings #43

Closed ntoxeg closed 3 years ago

ntoxeg commented 3 years ago

This is probably related to opencog/atomspace#2809

Traceback (most recent call last):
  File "examples/chase.py", line 13, in <module>
    from opencog.pln import *
ImportError: /usr/local/lib/opencog/libatom_types.so: undefined symbol: _ZN7opencog11ClassServer16update_factoriesEv
ntoxeg commented 3 years ago

Well, I have stumbled upon a clue: if you do from opencog.atomspace import * first, this problem does not occur. Also, spacetime is affected as well.

linas commented 3 years ago

Can this be fixed by performing the import in the pln bindings wrapper?

ntoxeg commented 3 years ago

Well, as a workaround yes, it would also have to happen in spacetime and potentially any other project (for URE though it is not needed). It would be nice to understand why this actually happens, but it is not a priority for sure.

linas commented 3 years ago

understand why this actually happens

I can explain that (partly): the atomspace libraries have circular dependencies among one-another, and, in general, have to all be used together to work. It would appear that the python bindings to PLN are linking to libatom_types.so but is not linking to libatombase.so where the ClassServer is defined. Look through the CMakefiles, make sure that ${ATOMSPACE_libraries} is being specified when building the python module.

ntoxeg commented 3 years ago

Look through the CMakefiles, make sure that ${ATOMSPACE_libraries} is being specified when building the python module.

Ah, here is the problem - variable names are case sensitive so ATOMSPACE_LIBRARIES is actually wrong. I will fix that IN PLN and spacetime then.

linas commented 3 years ago

ATOMSPACE_LIBRARIES

oh uh, ahh, I dunno, double-check; I may have written it wrong. I was typing it in off the top of my head, I don't remember the upper-lower case details I thought it was mixed-case...

ntoxeg commented 3 years ago

I will actually check if I can manage to make this work the "modern" way - you should not need to rely on variables for this nowadays. Hopefully it won't require too much work.

ntoxeg commented 3 years ago

Okay, so there is indeed a problem with circular dependencies - atom_types depends on atombase now but atombase has an already existing dependency on atom_types. It seems that this Python wrapper is correctly linking with AtomSpace but the error simply occurs because the aforementioned symbol is being looked up in atom_types, before other AtomSpace libraries are loaded. I guess the only option for now is to add from opencog.atomspace import * to the wrapper...

linas commented 3 years ago

Hang on .. I think I know a fix, its to the atomspace ...

linas commented 3 years ago

This change to the atomspace should fix this, I think:

--- a/lib/AtomSpaceConfig.cmake.in
+++ b/lib/AtomSpaceConfig.cmake.in
@@ -22,6 +22,11 @@ set_property(TARGET truthvalue
        execution
 )

+set_property(TARGET atom_types
+       APPEND PROPERTY INTERFACE_LINK_LIBRARIES
+       atombase
+)
+
 # These names are used in various projects e.g. opencog, as-moses, etc.
 # XXX FIXME ... they probably should not be! It would be best if the all
 # of the libs were specified i.e. LINK_LIBRARIES(${ATOMSPACE_LIBRARIES})

after applying above diff, rebuild, reinstall the atomspace, I think this will fix it. Sorry that this did not occur to me before!

Above is now in opencog/atomspace#2813

ntoxeg commented 3 years ago

Well, this is pretty much cheating around the fact that atombase and atom_types generate a lot of circular dependencies - no matter if I try to link atom_types to atombase as INTERFACE or PRIVATE, CMake will point out that:

[cmake] CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
[cmake]   "atom_types" of type SHARED_LIBRARY
[cmake]     depends on "atombase" (weak)
[cmake]     depends on "value" (weak)
[cmake]     depends on "truthvalue" (weak)
[cmake]   "value" of type SHARED_LIBRARY
[cmake]     depends on "atom_types" (weak)
[cmake]     depends on "atombase" (weak)
[cmake]     depends on "truthvalue" (weak)
[cmake]   "atombase" of type SHARED_LIBRARY
[cmake]     depends on "atom_types" (weak)
[cmake]     depends on "value" (weak)
[cmake]     depends on "truthvalue" (weak)
[cmake]     depends on "value" (strong)
[cmake]   "truthvalue" of type SHARED_LIBRARY
[cmake]     depends on "value" (weak)
[cmake]     depends on "atom_types" (weak)
[cmake]     depends on "atombase" (weak)
[cmake] At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.
[cmake] CMake Generate step failed.  Build files cannot be regenerated correctly.

That change of yours from above pretty much bypasses those safeguards and I don’t think this is worth doing - we can live for now with that from opencog.atomspace import * workaround for now.

The only actual fix is to actually deal with this interdependency between atom_types and atombase, you can see from that error message that they always occur together as dependencies in those other components - perhaps it’s time to combine them into one?

I think that merge you’ve incorporated into AtomSpace should be reverted too.

ntoxeg commented 3 years ago

Bad news, you can’t just add from opencog.atomspace import * in the module itself, you have to do it in your own Python session before loading the module itself. So for now all code using AtomSpace has to add that in the beginning.

linas commented 3 years ago

There are dozens of circular dependencies between different parts of the atomspace. Literally dozens. It used to be much much worse.

The cmake "safegaurd" is not a safegaurd at all, it is a basic design stupidity stemming from the fact that Windows is broken. So, rather than allowing common sense to reign in the Linux world, they decided to enforce the stupidity of Windows onto all other operating systems. This is reason 1001 why I hate Microsoft and Windows and all that they stand for; they are a giant pollution upon computer science and common sense.

ntoxeg commented 3 years ago

Well, this “fix” doesn’t work anyway:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: /usr/local/lib/opencog/libtruthvalue.so: undefined symbol: _ZN7opencog14EvaluationLink11do_evaluateEPNS_9AtomSpaceERKNS_6HandleEb
linas commented 3 years ago

this “fix” doesn’t work anyway:

Which fix?

ntoxeg commented 3 years ago

@linas the change to AtomSpace you have made, there is a different but analogical error now.

linas commented 3 years ago

there is a different but analogical error now.

What error is that? I assume the new undefined symbol? Again, you must make sure that the python bindings to PLN link to all of the atomspace libraries, and not just some of them.

I am wildly guessing that perhaps the -Wl,--no-as-needed linker flag is required to correctly link the PLN python bindings?

The most experienced python users are @noskill and @vsbogd -- perhaps they might be able to find the underlying bug the fastest?

linas commented 3 years ago

... anyway... on my system, when I do ldd -r /usr/local/lib/python3.7/dist-packages/opencog/pln.so I do not see any unresolved symbols. Can you check on your system?

In particular, are you sure that you do not have some other, old versions of pln.so that have this problem?

ntoxeg commented 3 years ago

@linas Ah, there we go! I have followed how it’s done in AtomSpace and added ${NO_AS_NEEDED} to the target_link_libraries command. In fact, the change you have introduced to AtomSpace earlier to solve this issue is not needed - this is the actual solution.

linas commented 3 years ago

Yayy!