Python feedback changes (#942)

- Some changes are for #913
- No longer require name argument on `Pipeline.to_file`
- Added `Pipeline.to_json_str`
- Added `Pipeline.name`
- Added `PipelineFilter.comments`
- `IFilter::toJson` now uses default values when the value in the arguments doesn't exist instead of failing
- Fixed the lifetime of docstrings generated at runtime
- Removed `registerPluginPyFilters`

Signed-off-by: Jared Duffey <[email protected]>

src/Plugins/ITKImageProcessing/wrapping/python/itkimageprocessing.cpp

@@ -16,6 +16,4 @@ PYBIND11_MODULE(itkimageprocessing, mod)
   auto* plugin = internals.addPlugin<ITKImageProcessingPlugin>();
 
   ITKImageProcessing::BindFilters(mod, internals);
-
-  internals.registerPluginPyFilters(*plugin);
 }

src/Plugins/OrientationAnalysis/wrapping/python/orientationanalysis.cpp

@@ -53,6 +53,4 @@ PYBIND11_MODULE(orientationanalysis, mod)
   internals.addConversion<OEMEbsdScanSelectionParameter>();
 
   OrientationAnalysis::BindFilters(mod, internals);
-
-  internals.registerPluginPyFilters(*plugin);
 }

src/Plugins/SimplnxCore/wrapping/python/simplnxpy.cpp

@@ -493,6 +493,7 @@ PYBIND11_MODULE(simplnx, mod)
   });
 
   py::class_<AbstractPipelineNode, std::shared_ptr<AbstractPipelineNode>> abstractPipelineNode(mod, "AbstractPipelineNode");
+  abstractPipelineNode.def("to_json_str", [](const AbstractPipelineNode& self) { return self.toJson().dump(); });
   py::class_<PipelineFilter, AbstractPipelineNode, std::shared_ptr<PipelineFilter>> pipelineFilter(mod, "PipelineFilter");
 
   py::class_<IParameter> parameter(mod, "IParameter");
@@ -1341,13 +1342,22 @@ PYBIND11_MODULE(simplnx, mod)
       "path"_a);
   pipeline.def(
       "to_file",
-      [](Pipeline& self, const std::string& name, const std::filesystem::path& path) {
+      [](const Pipeline& self, const std::filesystem::path& path) {
         nlohmann::json pipelineJson = self.toJson();
-        pipelineJson["name"] = name;
-        std::ofstream file(path, std::ios_base::binary);
-        file << pipelineJson;
+        std::ofstream file(path);
+        if(!file.is_open())
+        {
+          throw std::runtime_error(fmt::format("Failed to open '{}'", path.string()));
+        }
+        file << pipelineJson.dump(2) << "\n";
+        file.flush();
+        if(!file)
+        {
+          throw std::runtime_error(fmt::format("Failed to write pipeline json to '{}'", path.string()));
+        }
       },
-      "name"_a, "path"_a);
+      "path"_a);
+  pipeline.def_property("name", &Pipeline::getName, &Pipeline::setName);
   pipeline.def("execute", &ExecutePipeline);
   pipeline.def(
       "__getitem__", [](Pipeline& self, Pipeline::index_type index) { return self.at(index); }, py::return_value_policy::reference_internal);
@@ -1394,6 +1404,7 @@ PYBIND11_MODULE(simplnx, mod)
         return filter->humanName();
       },
       "Returns the human facing name of the filter");
+  pipelineFilter.def_property("comments", &PipelineFilter::getComments, &PipelineFilter::setComments);
 
   py::class_<PyFilter, IFilter> pyFilter(mod, "PyFilter");
   pyFilter.def(py::init<>([](py::object object) { return std::make_unique<PyFilter>(std::move(object)); }));
@@ -1451,13 +1462,21 @@ PYBIND11_MODULE(simplnx, mod)
 
   SimplnxCore::BindFilters(mod, *internals);
 
-  internals->registerPluginPyFilters(*corePlugin);
-
   mod.def("get_all_data_types", &GetAllDataTypes);
 
   mod.def("convert_numeric_type_to_data_type", &ConvertNumericTypeToDataType);
 
-  mod.def("get_filters", [internals, corePlugin]() { return internals->getPluginPyFilters(corePlugin->getId()); });
+  mod.def("get_filters", [corePlugin]() {
+    auto filterHandles = corePlugin->getFilterHandles();
+    std::vector<py::type> filterList;
+    for(const auto& handle : filterHandles)
+    {
+      py::object filter = py::cast(corePlugin->createFilter(handle.getFilterId()));
+      py::type filterType = py::type::of(filter);
+      filterList.push_back(filterType);
+    }
+    return filterList;
+  });
 
   mod.def("test_filter", [](const IFilter& filter) { return py::make_tuple(filter.uuid(), filter.name(), filter.humanName(), filter.className(), filter.defaultTags()); });
 

src/simplnx/Filter/IFilter.cpp

@@ -251,7 +251,15 @@ nlohmann::json IFilter::toJson(const Arguments& args) const
   Parameters params = parameters();
   for(const auto& [name, param] : params)
   {
-    nlohmann::json parameterJson = param->toJson(args.at(name));
+    nlohmann::json parameterJson;
+    if(args.contains(name))
+    {
+      parameterJson = param->toJson(args.at(name));
+    }
+    else
+    {
+      parameterJson = param->toJson(param->defaultValue());
+    }
     json[name] = std::move(parameterJson);
   }
   return json;

wrapping/python/CxPybind/CxPybind/CxPybind.hpp

@@ -234,25 +234,6 @@ class Internals
     return addPlugin(std::make_shared<T>());
   }
 
-  void registerPluginPyFilters(const AbstractPlugin& plugin)
-  {
-    // Must be called after all types are registered
-    std::vector<py::type> list;
-    for(auto handle : plugin.getFilterHandles())
-    {
-      py::object filter = py::cast(plugin.createFilter(handle.getFilterId()));
-      py::type filterType = py::type::of(filter);
-      list.push_back(filterType);
-    }
-
-    m_PluginFilterMap.insert({plugin.getId(), list});
-  }
-
-  const std::vector<py::type>& getPluginPyFilters(const Uuid& pluginUuid)
-  {
-    return m_PluginFilterMap.at(pluginUuid);
-  }
-
   void loadPythonPlugin(py::module_& mod);
 
   PythonPlugin* getPythonPlugin(const Uuid& id)
@@ -281,7 +262,6 @@ class Internals
 
 private:
   std::unordered_map<Uuid, PyParameterInfo> m_ParameterConversionMap;
-  std::unordered_map<Uuid, std::vector<py::type>> m_PluginFilterMap;
   std::unordered_map<Uuid, std::shared_ptr<PythonPlugin>> m_PythonPlugins;
   std::shared_ptr<Application> m_App;
 };
@@ -394,6 +374,16 @@ inline IFilter::MessageHandler CreatePyMessageHandler()
   return IFilter::MessageHandler{&PyPrintMessage};
 }
 
+template <class T>
+void CreatePythonCleanupCallback(py::handle object, std::unique_ptr<T> holder)
+{
+  py::cpp_function nameCleanupCallback([holder = std::move(holder)](py::handle weakref) mutable {
+    holder.reset();
+    weakref.dec_ref();
+  });
+  py::weakref(object, nameCleanupCallback).release();
+}
+
 template <class FilterT>
 auto BindFilter(py::handle scope, const Internals& internals)
 {
@@ -407,7 +397,7 @@ auto BindFilter(py::handle scope, const Internals& internals)
     options.disable_function_signatures();
 
     std::string executeSig = MakePythonSignature<FilterT>("execute", internals);
-    std::string executeDocString = fmt::format("{}\n\nExecutes the filter\n", executeSig);
+    auto executeDocStringHolder = std::make_unique<std::string>(fmt::format("{}\n\nExecutes the filter\n", executeSig));
     filter.def_static("human_name", [&internals]() {
       FilterT filter;
       return filter.humanName();
@@ -441,7 +431,8 @@ auto BindFilter(py::handle scope, const Internals& internals)
           IFilter::ExecuteResult result = filter.execute(dataStructure, convertedArgs, nullptr, CreatePyMessageHandler());
           return result;
         },
-        "data_structure"_a, executeDocString.c_str());
+        "data_structure"_a, executeDocStringHolder->c_str());
+    CreatePythonCleanupCallback(filter, std::move(executeDocStringHolder));
 
     // std::string preflightSig = MakePythonSignature<FilterT>("preflight", internals);
     // std::string preflightDocString = fmt::format("{}\n\nExecutes the filter\n", sig);

wrapping/python/examples/scripts/pipeline.py

@@ -52,15 +52,11 @@
 
 pipeline = nx.Pipeline().from_file(nxtest.get_simplnx_source_dir() / 'src/Plugins/OrientationAnalysis/pipelines/EBSD Reconstruction/(01) Small IN100 Archive.d3dpipeline')
 
-pipeline.to_file( "test pipeline", nxtest.get_test_temp_directory() / "python_pipeline.d3dpipeline")
-
-
+pipeline.to_file(nxtest.get_test_temp_directory() / "python_pipeline.d3dpipeline")
 
 # pipeline.append(nx.CreateDataArrayFilter(), {'numeric_type_index': nx.NumericType.int32})
 # pipeline[0].set_args({'numeric_type_index': nx.NumericType.int32})
 
 # did_execute = pipeline.execute(data_structure)
 
 # print('Pipeline Execute: {}'.format(did_execute))
-
-