SoarGroup / Soar

Soar, a general cognitive architecture for systems that exhibit intelligent behavior.
http://soar.eecs.umich.edu
Other
321 stars 70 forks source link

WMEs can be duplicated if added over SML #456

Open ShadowJonathan opened 1 month ago

ShadowJonathan commented 1 month ago

I encountered this bug today:

import soar_sml

k=soar_sml.Kernel.CreateKernelInNewThread()
a=k.CreateAgent('soar')

inp = a.GetInputLink()

inp.CreateStringWME("one", "hi")
inp.CreateStringWME("one", "hi")

inp.CreateFloatWME("one", 1.0)
inp.CreateFloatWME("one", 1.0)

inp.CreateIntWME("one", 1)
inp.CreateIntWME("one", 1)

i = inp.CreateIdWME("id")

inp.CreateSharedIdWME("one", i)
inp.CreateSharedIdWME("one", i)

a.RunSelf(1)

print(a.ExecuteCommandLine("print -t --internal I2"))
print(a.ExecuteCommandLine("print -t I2"))

Where adding multiple concrete-value WMEs result in them being duplicated:

(20: I2 ^id I4)
(21: I2 ^one I4)
(19: I2 ^one 1)
(18: I2 ^one 1)
(17: I2 ^one 1.000000)
(16: I2 ^one 1.000000)
(15: I2 ^one hi)
(22: I2 ^one I4)
(14: I2 ^one hi)

(I2 ^id I4)
(I2 ^one I4)
(I2 ^one 1)
(I2 ^one 1)
(I2 ^one 1.000000)
(I2 ^one 1.000000)
(I2 ^one hi)
(I2 ^one I4)
(I2 ^one hi)

(@scijones says that this could possibly result to internal segfaults 😬)

garfieldnate commented 1 month ago

Did @scijones see this output when he made the comment? This looks okay to me, but I could be wrong.

scijones commented 1 month ago

Useless comment:

_I saw the printout.

The problem, as I see it, is that the print makes it seem as though there are truly two WMEs with the same attribute label and same value from the same parent id. Soar supports multi-valued attributes, sure, but it doesn't support multi-[the-same-value] attributes. (Consider when two rules provide support for the same WME. Soar does not create a duplicate. It just maintains that WME if either rule remains to provide support.)_

scijones commented 1 month ago

oh shoot, timetags. I missed the "-t". duh. I need to see it without the "-t" for sure.

scijones commented 1 month ago

Okay, I did it myself and it's a real issue:

print(a.ExecuteCommandLine("print I2 -d 3")) (I2 ^one hi ^one hi)

garfieldnate commented 1 month ago

Soar supports multi-valued attributes, sure, but it doesn't support multi-[the-same-value] attributes

This is interesting. So you're saying that (I2 ^one hi bye) is legal but (I2 ^one hi hi) is not. I don't know how Soar de-duplicates triples now, but the IO links have special machinery attached to them, so that's somewhere for me to look.

ShadowJonathan commented 1 month ago

(FWIW I don't exactly know what the SML bindings should respond with wrt this, if it should reply with the same identifier, or if it should respond with NULL)

ShadowJonathan commented 1 month ago

Something else of note, i found this piece of code in CreateSharedIdWME which does check if the WME about to be added will duplicate the set:

https://github.com/soargroup/soar/blob/f5a59643edb0551d668ea0b36ada8a788080e9ba/Core/ClientSML/src/sml_ClientWorkingMemory.cpp#L1194-L1206

However, the == there will miss certain created shared Ids, since i know that (at least) on every GetChild call, SML will create a new Identifier pointer, and that == would only be checking on pointer equality.

(Curiously, GetChild does not copy constant-value WMEs, those always return the same pointer)

garfieldnate commented 1 month ago

@ShadowJonathan where are you seeing the GetChild call?

T_T Wish I could look at the original discussion in the ticket mentioned in the code, but I think it's all gone (was on google code).

ShadowJonathan commented 1 month ago

Ah, this is with my own library code, but this should replicate it;

import soar_sml

k=soar_sml.Kernel.CreateKernelInNewThread()
a=k.CreateAgent('soar')

inp = a.GetInputLink()

i = inp.CreateIdWME("id")

i2 = inp.GetChild(0)

assert(i != i2)

print((repr(i), i.GetTimeTag()))
print((repr(i2), i2.GetTimeTag()))

It should say the two aren't the "same", and the representation of both should point to a different pointer.

(Unfortunately, DebugString doesn't work in the default bindings, because SWIG generates code that requires (paraphrasing from memory) a std::string &, which python cannot create; all strings are values; interned and constant)

Edit: These two should probably be separate issues, tell me if I should file them like that ^^

garfieldnate commented 1 month ago

Thank you! We can springboard off of this to write a unit test in Soar. Then we can fix the original issue for CreateSharedIdWME. That still leaves the string, float, and int cases.