Various Python bindings fixes

Migrate Python to nicer C++ RHS/Client Message handler interfaces. Replace the Python 2 PyString_Check and PyString_AsString with Python 3 PyUnicode_Check and PyUnicode_AsUTF8. This actually got rid of a segfault for me. Update the Python SML tests; there's more that could be added, but this is some basic testing functionality for various handler registrations. Add the Python SML tests to CI.
SoarGroup · Aug 31, 2023 · 65450e9 · 65450e9
1 parent d7aece7
commit 65450e9
Show file tree

Hide file tree

Showing 8 changed files with 203 additions and 123 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -61,12 +61,18 @@ jobs:
         # TODO: why do these fail? Make them pass.
         run: ./UnitTests -e PRIMS_Sanity1 -e PRIMS_Sanity2 -f testLoadLibrary -f testSmemArithmetic
 
-      - name: SML client tests
+      - name: SML Java tests
         working-directory: ./out
         env:
           LD_LIBRARY_PATH: ${{ github.workspace }}/out
         run: java -jar java/soar-smljava.jar
-        # TODO: run additional tests for Python, Tcl, CSharp
+
+      - name: SML Python tests
+        working-directory: ./out
+        env:
+          LD_LIBRARY_PATH: ${{ github.workspace }}/out
+        run: python3 TestPythonSML.py
+        # TODO: run additional tests for Tcl, CSharp
 
       # reports JUnit test results as GitHub PR check.
       - name: publish test report
@@ -159,14 +165,22 @@ jobs:
              throw "UnitTests exit code: $lastexitcode"
           }
 
-      - name: SML client tests
+      - name: SML Java tests
         working-directory: ./out
         run: |
           java -jar java/soar-smljava.jar
           if ($lastexitcode -ne 0) {
              throw "soar-smljava exit code: $lastexitcode"
           }
-        # TODO: run additional tests for Python, Tcl, CSharp
+
+      - name: SML Python tests
+        working-directory: ./out
+        run: |
+          python3 TestPythonSML.py
+          if ($lastexitcode -ne 0) {
+             throw "soar-smljava exit code: $lastexitcode"
+          }
+        # TODO: run additional tests for Tcl, CSharp
 
       # reports JUnit test results as GitHub PR check.
       - name: publish test report

diff --git a/Core/ClientSML/src/sml_ClientAgent.cpp b/Core/ClientSML/src/sml_ClientAgent.cpp
@@ -255,6 +255,7 @@ void Agent::ReceivedPrintEvent(smlPrintEventId id, AnalyzeXML* pIncoming, Elemen
 void Agent::ReceivedProductionEvent(smlProductionEventId id, AnalyzeXML* pIncoming, ElementXML* /*pResponse*/)
 {
     char const* pProductionName = pIncoming->GetArgString(sml_Names::kParamName) ;
+    // TODO: this is always 0, so remove it
     char const* pInstance = 0 ; // gSKI defines this but doesn't support it yet.
 
     // Look up the handler(s) from the map

diff --git a/Core/ClientSML/src/sml_ClientKernel.cpp b/Core/ClientSML/src/sml_ClientKernel.cpp
@@ -1896,6 +1896,7 @@ class Kernel::TestRhsCallbackFull : public RhsEventMap::ValueTest
 *************************************************************/
 int Kernel::RegisterForSystemEvent(smlSystemEventId id, SystemEventHandler handler, void* pUserData, bool addToBack)
 {
+    std::cerr << "Kernel::RegisterForSystemEvent" << std::endl ;
     // Start by checking if this id, handler, pUSerData combination has already been registered
     TestSystemCallbackFull test(id, handler, pUserData) ;
 

diff --git a/Core/ClientSMLSWIG/Python/Python_sml_ClientInterface.i b/Core/ClientSMLSWIG/Python/Python_sml_ClientInterface.i
@@ -43,13 +43,12 @@
 	std::list<PythonUserData*> callbackdatas;
 
 	void show_exception_and_exit(const char *type, int id) {
-		std::cerr << "Uncaught Python exception in " << type << " callback id = " << id << ". Exiting." << std::endl;
+		PyTraceBack_Here(PyEval_GetFrame());
+        std::cerr << "Uncaught Python exception in " << type << " callback id = " << id << ". Exiting." << std::endl;
 		PyObject *excType, *excValue, *excTraceback;
 		PyErr_Fetch(&excType, &excValue, &excTraceback);
 		PyErr_NormalizeException(&excType, &excValue, &excTraceback);
-		char buf[7];
-		strcpy(buf, "stderr");
-		PyTraceBack_Print(excTraceback, PySys_GetObject(buf));
+		PyTraceBack_Print(excTraceback, PySys_GetObject("stderr"));
 		exit(1);
 	}
 
@@ -191,6 +190,7 @@
 
 	void PythonSystemEventCallback(sml::smlSystemEventId id, void* pUserData, sml::Kernel* pKernel)
 	{
+        std::cerr << "PythonSystemEventCallback1: " << id << std::endl;
 		PyGILState_STATE gstate;
 		gstate = PyGILState_Ensure(); /* Get the thread.  No Python API allowed before this point. */
 
@@ -248,11 +248,11 @@
 		Py_DECREF(args);
 		if(!result) {
 			show_exception_and_exit("string event", id);
-		} else if (!PyString_Check(result)) {
+		} else if (!PyUnicode_Check(result)) {
 			return "";
 		}
 
-		std::string res = PyString_AsString(result);
+		std::string res = PyUnicode_AsUTF8 (result);
 		Py_DECREF(result);
 
 		PyGILState_Release(gstate); /* Release the thread. No Python API allowed beyond this point. */
@@ -282,20 +282,8 @@
 		PyGILState_Release(gstate); /* Release the thread. No Python API allowed beyond this point. */
 	}
 
-	const char *PythonRhsEventCallback(sml::smlRhsEventId id, void* pUserData, sml::Agent* pAgent, char const* pFunctionName, char const* pArgument, int *bufSize, char *buf)
+	const std::string PythonRhsEventCallback(sml::smlRhsEventId id, void* pUserData, sml::Agent* pAgent, char const* pFunctionName, char const* pArgument)
 	{
-        // Previous result was cached, meaning client should be calling again to get it
-        // return that result and clear the cache
-        static std::string prevResult;
-        if ( !prevResult.empty() )
-        {
-            strncpy( buf, prevResult.c_str(), *bufSize );
-
-            prevResult = "";
-
-            return buf;
-        }
-
 	    PyGILState_STATE gstate;
 		gstate = PyGILState_Ensure(); /* Get the thread.  No Python API allowed before this point. */
 
@@ -310,29 +298,27 @@
 		Py_DECREF(args);
 		if(!result) {
 			show_exception_and_exit("RHS event", id);
-		} else if (!PyString_Check(result)) {
+		} else if (!PyUnicode_Check(result)) {
 			return "";
 		}
 
-		std::string res = PyString_AsString(result);
+		std::string res = PyUnicode_AsUTF8 (result);
 		Py_DECREF(result);
 
 		PyGILState_Release(gstate); /* Release the thread. No Python API allowed beyond this point. */
 
-        // Too long to fit in the buffer; cache result and signal client with
-        // NULL return value to call again with a larger buffer
-        if ( res.length() + 1 > *bufSize )
-        {
-            *bufSize = res.length() + 1;
-            prevResult = res;
-            return NULL;
-        }
-        strcpy( buf, res.c_str() );
-
-        return buf;
+        return res;
 	}
 
-	const char *PythonClientMessageEventCallback(sml::smlRhsEventId id, void* pUserData, sml::Agent* pAgent, char const* pClientName, char const* pMessage, int *bufSize, char *buf)
+    const sml::RhsEventHandlerCpp getPythonRhsEventCallback(void *pUserData)
+    {
+        return [pUserData](sml::smlRhsEventId id, sml::Agent *pAgent, char const *pFunctionName, char const *pArgument) -> const std::string
+        {
+            return PythonRhsEventCallback(id, pUserData, pAgent, pFunctionName, pArgument);
+        };
+    }
+
+    const char *PythonClientMessageEventCallback(sml::smlRhsEventId id, void* pUserData, sml::Agent* pAgent, char const* pClientName, char const* pMessage, int *bufSize, char *buf)
 	{
         // Previous result was cached, meaning client should be calling again to get it
         // return that result and clear the cache
@@ -359,11 +345,11 @@
 		Py_DECREF(args);
 		if(!result) {
 			show_exception_and_exit("client message event", id);
-		} else if (!PyString_Check(result)) {
+		} else if (!PyUnicode_Check(result)) {
 			return "";
 		}
 
-		std::string res = PyString_AsString(result);
+		std::string res = PyUnicode_AsUTF8 (result);
 		Py_DECREF(result);
 
 		PyGILState_Release(gstate); /* Release the thread. No Python API allowed beyond this point. */
@@ -520,7 +506,8 @@
 %extend sml::Kernel {
 
     long RegisterForSystemEvent(sml::smlSystemEventId id, PyObject* func, PyObject* userData, bool addToBack = true) {
-	    PythonUserData* pud = CreatePythonUserData(func, userData);
+        std::cerr << "RegisterForSystemEvent: " << id << std::endl;
+        PythonUserData* pud = CreatePythonUserData(func, userData);
 	    pud->callbackid = self->RegisterForSystemEvent(id, PythonSystemEventCallback, (void*)pud, addToBack);
 	    return (long)pud;
     }
@@ -545,7 +532,7 @@
 
     long AddRhsFunction(char const* pRhsFunctionName, PyObject* func, PyObject* userData, bool addToBack = true) {
 	    PythonUserData* pud = CreatePythonUserData(func, userData);
-	    pud->callbackid = self->AddRhsFunction(pRhsFunctionName, PythonRhsEventCallback, (void*)pud, addToBack);
+        pud->callbackid = self->AddRhsFunction(pRhsFunctionName, getPythonRhsEventCallback((void*)pud), addToBack);
 	    return (long)pud;
     }
 

diff --git a/Core/ClientSMLSWIG/Python/SConscript b/Core/ClientSMLSWIG/Python/SConscript
@@ -57,5 +57,6 @@ else:
     shlib = clone.SharedLibrary(name, interface, SHLIBPREFIX = '_')
 install_source = env.Install(lib_install_dir, source)
 install_lib = env.Install(lib_install_dir, shlib)
+install_test = env.Install(lib_install_dir, env.File('TestPythonSML.py'))
 
-env.Alias(python_sml_alias, install_lib + install_source)
+env.Alias(python_sml_alias, install_lib + install_source + install_test)