opencog::attentionbank(...) should not rely that objects compared by references are equal

[stellarspot]

opencog::attentionbank(AtomSpace* asp) method in AttentionBank has the following check to compare if two AtomSpaces are equal:

https://github.com/opencog/opencog/blob/802f0ec84e90db55389ee8ace40be6004a1ec9d1/opencog/attentionbank/bank/AttentionBank.cc#L249

This can fail if one AtomSpace is deleted and immediately after that the second one is created.
because the the new AtomSpace can be created with the same memory address as the previous one.

I encountered it in the Python test that can or can't fail depending with each address the new AtomSpace is created:
(note that the opencog/opencog#3549 pull request is required to run the test)

import unittest
from unittest import TestCase

from opencog.type_constructors import *
from opencog.utilities import initialize_opencog, finalize_opencog
from opencog.bank import AttentionBank, af_bindlink


class AttentionBankTest(TestCase):

    def setUp(self):
        self.atomspace = AtomSpace()
        initialize_opencog(self.atomspace)

    def tearDown(self):
        finalize_opencog()
        del self.atomspace

    def test_get_handles_by_av_different_as1(self):
        attention_bank = AttentionBank(self.atomspace)

        node1 = ConceptNode("node1")
        attention_bank.set_sti(node1, 2.0)

        atoms = attention_bank.get_atoms_by_av(1.0, 4.0)
        self.assertEqual(1, len(atoms))

    def test_get_handles_by_av_different_as2(self):
        attention_bank = AttentionBank(self.atomspace)

        node2 = ConceptNode("node2")
        attention_bank.set_sti(node2, 2.0)

        atoms = attention_bank.get_atoms_by_av(1.0, 4.0)
        self.assertEqual(1, len(atoms))


if __name__ == '__main__':
    unittest.main()

[linas]

There is no maintainer for the attentionbank code. Patches invited :-)

[chamorajg]

Can I work on this issue. I would like to contribute for this framework.

[linas]

Can I work on this issue. I would like to contribute for this framework.

That would be excellent! Each atomspace gets it's own new, unique UUID (use the c++ get_uuid() call) I imagine that is enough to fix this.

[chamorajg]

So the _instance variable which is created after deleting needs to have it's own unique UUID right ?

[linas]

The AtomSpace API can be found in the header file AtomSpace.h. Additional documentation for what the various methods do can be found in the AtomSpace.cc and the AtomTable.cc files.

[chamorajg]

We should use get_uuid() to _as variable before checking the condition right ? So that _as doesn't _as doesn't point to _asp. I am new to this code so I am a bit confused a little help would do.

[linas]

Yes, I guess so.. something like that.

BTW, I plan to split out the attention/attentionbank code into it's own git repo in the next few days or weeks, so please be prepared for that change.

[chamorajg]

I just updated the source code locally. I need to compile it and test it locally once before committing any changes. So how do I compile the source code. I have gone through multiple wiki pages but it's a bit confusing to find out.

[stellarspot]

You need to use cmake to compile the opencog project.
Note, that cogutil and atomspace projects needs to be installed before opencog building.
For example:

# install cogutil
git clone https://github.com/opencog/cogutil
mkdir -p cogutil/build
pushd
cd cogutil/build
cmake ..
make -j4
sudo make -j4 install
popd

# install atomspace
git clone https://github.com/opencog/atomspace
mkdir -p atomspace/build
pushd
cd atomspace/build
cmake ..
make -j4
sudo make -j4 install
popd

# build opencog
git clone https://github.com/opencog/opencog
cd opencog
# create branch with fix
git checkout -b fix-issue-3550
# build opencog
mkdir build
cd build
cmake ..
make -j4
# run all tests
make -j4 test
# run specific attention bank test
ARGS='-R BankImplUTest' make test

[stellarspot]

Opencog project contains CircleCI config which can give a hint how to run the build process in docker:
https://github.com/opencog/opencog/blob/master/.circleci/config.yml

[chamorajg]

thanks @stellarspot for your resources I will go through them.

[chamorajg]

I updated the source code to have unique UUID to the variable and tried the test multiple times. It passed every time I executed the BankImplUTest.cpp code. Should I try it a multiple times again. @linas @stellarspot Need your suggestions on this.

[stellarspot]

You need to create a fork of opencog project, create there a branch, apply your fix, push it to the forked repository and create a pull request to the original opencog project.

[chamorajg]

Can't we just compare the addresses of _as and asp using & operator inside the if condition ? When we write the condition _as->get_uuid() != asp->get_uuid() inside the if statement we get an error saying since static objects like _as cannot access const functions like get_uuid() . So won't it be the same if we use & before the _as and asp objects and then compare them. Will this condition suffice to check if they have the same memory address or not if ( &_as != &asp and _instance). @linas will this work ?

[linas]

I don't understand the previous comment. Normally, code refers to the atomspace with a pointer, and not as a reference. Also, in C++, the static keyword does not prevent methods from being called.

[chamorajg]

@linas I will try this once again and try fix this issue soon.