Create PythonObjectProperty
#39880
Create PythonObjectProperty
#39880
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new PythonObjectProperty type to the PythonInterface core, including public header and implementation, and wires it into CMake. Introduces Kernel-side aliases/specializations for boost::python::object and explicit instantiation of PropertyWithValue. Implements PythonObjectProperty methods (default/isDefault; setValue/JSON throw NotImplementedError). Updates PropertyWithValueExporter to use if constexpr for std::string. Adds Boost.Python exports for PythonObjectProperty, registers a PythonObjectTypeHandler in the type registry, and implements its create method. Updates kernel CMake to include new export and registry sources. Adds corresponding Python unit tests and includes them in the test suite. Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
Framework/PythonInterface/core/inc/MantidPythonInterface/core/PropertyWithValueExporter.h (2)
22-22: Switch to if constexpr is correct; add missing standard headers.
This file uses std::is_same, std::stringstream, and std::string but doesn’t include their headers locally.Apply:
#include <boost/python/return_value_policy.hpp> #endif + +// local standard deps +#include <type_traits> +#include <sstream> +#include <string>
23-27: Minor: avoid copy and stringstream in dtype for std::string.
Reduces allocations and improves clarity.- std::stringstream ss; - std::string val = self.value(); - ss << "S" << val.size(); - std::string ret_val = ss.str(); - return ret_val; + const auto len = self.value().size(); + return "S" + std::to_string(len);Framework/PythonInterface/mantid/kernel/CMakeLists.txt (1)
83-90: Add PythonObjectTypeHandler.h to INC_FILES for consistency and IDE visibility.
Currently other registry headers are listed; include the new one as well.Proposed addition within the existing INC_FILES block:
inc/MantidPythonInterface/kernel/Registry/PythonObjectTypeHandler.hFramework/PythonInterface/test/python/mantid/kernel/PythonObjectPropertyTest.py (2)
7-7: Fix incorrect module docstring.Rename to reflect the class under test.
-"""Test the exposed PropertyManagerProperty""" +"""Tests for PythonObjectProperty"""
22-24: Remove unused stubmy_serializer.It’s dead code in this test module.
-def my_serializer(): - pass -Framework/PythonInterface/mantid/kernel/inc/MantidPythonInterface/kernel/Registry/PythonObjectTypeHandler.h (1)
9-10: Add explicit include for boost::python::object.Prevents reliance on include order and avoids accidental ODR surprises.
#include "MantidPythonInterface/kernel/Registry/TypedPropertyValueHandler.h" +#include <boost/python/object.hpp>Framework/PythonInterface/core/src/PythonObjectProperty.cpp (3)
32-34: MakeencodeAsJsona proper template specialization (consistency with toString).Declare it as a specialization to avoid unintended overloading/hiding.
-Json::Value encodeAsJson(PythonObject const &) { +template <> Json::Value encodeAsJson(PythonObject const &) { throw Exception::NotImplementedError("encodeAsJson(const boost::python::object &value)"); }
40-41: Remove unused using-declaration.
DataItemisn’t referenced.-using Kernel::DataItem;
54-56: Explicitly unsupported string/JSON setters — document externally.These throw by design; ensure public docs/examples don’t suggest string/JSON initialization for this property.
Also applies to: 64-66
Framework/PythonInterface/mantid/kernel/src/Exports/PythonObjectProperty.cpp (1)
30-34: Droparg("self")from constructor signatures.Boost.Python
__init__doesn’t takeself; keeping it can misdocument signatures.- .def(init<const std::string &, const unsigned int>( - (arg("self"), arg("name"), arg("direction") = Direction::Input), "Construct a PythonObjectProperty")) + .def(init<const std::string &, const unsigned int>( + (arg("name"), arg("direction") = Direction::Input), "Construct a PythonObjectProperty")) .def(init<const std::string &, const boost::python::object &, const unsigned int>( - (arg("self"), arg("name"), arg("defaultValue"), arg("direction") = Direction::Input), + (arg("name"), arg("defaultValue"), arg("direction") = Direction::Input), "Construct a PythonObjectProperty with a default value"));Framework/PythonInterface/core/inc/MantidPythonInterface/core/PythonObjectProperty.h (1)
80-98: Remove the string-based constructor.String→PythonObject parsing isn’t supported (the corresponding setters throw), so this ctor is misleading and risks runtime errors if used.
- /** Constructor from which you can set the property's values through a string: - * - * Inherits from the constructor of PropertyWithValue specifically made to - * handle a list - * of numeric values in a string format so that initial value is set - * correctly. - * - * @param name :: The name to assign to the property - * @param strvalue :: A string which will set the property being stored - * @param validator :: The validator to use for this property, if required - * @param direction :: The direction (Input/Output/InOut) of this property - * @throw std::invalid_argument if the string passed is not compatible with - * the array type - */ - PythonObjectProperty(std::string const &name, std::string const &strvalue, - IValidator_sptr const &validator = std::make_shared<NullValidator>(), - unsigned int const direction = Direction::Input) - : PropertyWithValue<PythonObject>(name, PythonObject(), strvalue, validator, direction) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
Framework/PythonInterface/core/CMakeLists.txt(2 hunks)Framework/PythonInterface/core/inc/MantidPythonInterface/core/PropertyWithValueExporter.h(1 hunks)Framework/PythonInterface/core/inc/MantidPythonInterface/core/PythonObjectProperty.h(1 hunks)Framework/PythonInterface/core/src/PythonObjectProperty.cpp(1 hunks)Framework/PythonInterface/mantid/kernel/CMakeLists.txt(2 hunks)Framework/PythonInterface/mantid/kernel/inc/MantidPythonInterface/kernel/Registry/PythonObjectTypeHandler.h(1 hunks)Framework/PythonInterface/mantid/kernel/src/Exports/PythonObjectProperty.cpp(1 hunks)Framework/PythonInterface/mantid/kernel/src/Registry/PythonObjectTypeHandler.cpp(1 hunks)Framework/PythonInterface/test/python/mantid/kernel/CMakeLists.txt(1 hunks)Framework/PythonInterface/test/python/mantid/kernel/PythonObjectPropertyTest.py(1 hunks)
🔇 Additional comments (12)
Framework/PythonInterface/test/python/mantid/kernel/CMakeLists.txt (1)
38-38: Added PythonObjectPropertyTest to suite — looks good.
Order relative to existing tests is consistent with the surrounding list.Framework/PythonInterface/core/CMakeLists.txt (2)
10-10: Core wiring for PythonObjectProperty added — OK.
Source is in SRC_FILES and public header in INC_FILES; build/link structure remains consistent.
40-40: Header listed in INC_FILES — OK.
Ensures IDE visibility and installation via existing patterns for this target.Framework/PythonInterface/mantid/kernel/CMakeLists.txt (2)
52-52: Export for PythonObjectProperty added in correct order — OK.
It appears after PropertyWithValue exports, which it depends on.
78-80: Registry source added — OK.
Ordering with other handlers is sensible.Framework/PythonInterface/test/python/mantid/kernel/PythonObjectPropertyTest.py (2)
27-36: Direction semantics tests look good.Covers default and explicit Direction paths.
37-54: Defaulting behavior tests look good.Validates name, Direction, and default None semantics.
Framework/PythonInterface/mantid/kernel/inc/MantidPythonInterface/kernel/Registry/PythonObjectTypeHandler.h (1)
28-30: LGTM: forwarding setter.Forwarding to
IPropertyManager::setPropertyforboost::python::objectis appropriate.Framework/PythonInterface/core/src/PythonObjectProperty.cpp (2)
17-27: Confirm no callers expect stringification.Both
toStringandtoPrettyStringthrow. Please confirm no UI/logging paths attempt to stringify this type.
71-71: LGTM: default detection.
isDefault()usingm_value.is_none()matches the intended semantics.Framework/PythonInterface/mantid/kernel/src/Exports/PythonObjectProperty.cpp (1)
24-26: LGTM: value-wrapper export and type-handler registration.Wrapper and TypeRegistry subscription for
boost::python::objectlook correct.Also applies to: 37-38
Framework/PythonInterface/core/inc/MantidPythonInterface/core/PythonObjectProperty.h (1)
48-61: Constructors and defaults look good.Covers None/defaulted and explicit-default use cases with/without validators.
Also applies to: 68-79
| #include "MantidPythonInterface/kernel/Registry/PythonObjectTypeHandler.h" | ||
| #include "MantidKernel/IPropertyManager.h" | ||
| #include "MantidPythonInterface/core/PythonObjectProperty.h" | ||
| #include <boost/python/extract.hpp> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include IsNone and GIL helper to make Python C-API usage safe.
This TU calls boost::python utilities; ensure symbols and GIL handling are available.
#include "MantidPythonInterface/kernel/Registry/PythonObjectTypeHandler.h"
#include "MantidKernel/IPropertyManager.h"
#include "MantidPythonInterface/core/PythonObjectProperty.h"
+#include "MantidPythonInterface/core/IsNone.h"
+#include "MantidPythonInterface/core/GlobalInterpreterLock.h"
+#include <stdexcept>
#include <boost/python/extract.hpp>🤖 Prompt for AI Agents
In
Framework/PythonInterface/mantid/kernel/src/Registry/PythonObjectTypeHandler.cpp
around lines 10 to 14, the TU uses Boost.Python utilities but does not include
the headers that provide IsNone and the GIL RAII helper; add the appropriate
header(s) that define IsNone and the Python GIL guard used across the project,
then ensure any use of Python C/API or boost::python extract/operations is
wrapped with the GIL (create a local RAII GIL acquire scope at function entry
where Python objects are accessed) so Python API usage is safe.
| const unsigned int direction) const { | ||
| using boost::python::extract; | ||
| using Kernel::IValidator; | ||
|
|
||
| std::unique_ptr<Kernel::Property> valueProp; | ||
| if (isNone(validator)) { | ||
| valueProp = std::make_unique<PythonObjectProperty>(name, defaultValue, direction); | ||
| } else { | ||
| const IValidator *propValidator = extract<IValidator *>(validator); | ||
| valueProp = std::make_unique<PythonObjectProperty>(name, defaultValue, propValidator->clone(), direction); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Acquire GIL and validate validator extraction to avoid crashes.
Calling boost::python::extract without the GIL or dereferencing a null/invalid IValidator* risks UB/crash.
std::unique_ptr<Kernel::Property> PythonObjectTypeHandler::create(std::string const &name,
boost::python::object const &defaultValue,
boost::python::object const &validator,
const unsigned int direction) const {
using boost::python::extract;
using Kernel::IValidator;
+ // Ensure GIL for all Boost.Python interactions in this scope
+ GlobalInterpreterLock gil;
+
std::unique_ptr<Kernel::Property> valueProp;
if (isNone(validator)) {
valueProp = std::make_unique<PythonObjectProperty>(name, defaultValue, direction);
} else {
- const IValidator *propValidator = extract<IValidator *>(validator);
- valueProp = std::make_unique<PythonObjectProperty>(name, defaultValue, propValidator->clone(), direction);
+ const auto extractor = extract<IValidator *>(validator);
+ if (!extractor.check() || extractor() == nullptr) {
+ throw std::invalid_argument("PythonObjectTypeHandler: validator must be a Mantid Kernel::IValidator");
+ }
+ const IValidator *propValidator = extractor();
+ valueProp =
+ std::make_unique<PythonObjectProperty>(name, defaultValue, propValidator->clone(), direction);
}
return valueProp;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const unsigned int direction) const { | |
| using boost::python::extract; | |
| using Kernel::IValidator; | |
| std::unique_ptr<Kernel::Property> valueProp; | |
| if (isNone(validator)) { | |
| valueProp = std::make_unique<PythonObjectProperty>(name, defaultValue, direction); | |
| } else { | |
| const IValidator *propValidator = extract<IValidator *>(validator); | |
| valueProp = std::make_unique<PythonObjectProperty>(name, defaultValue, propValidator->clone(), direction); | |
| } | |
| std::unique_ptr<Kernel::Property> PythonObjectTypeHandler::create(std::string const &name, | |
| boost::python::object const &defaultValue, | |
| boost::python::object const &validator, | |
| const unsigned int direction) const { | |
| using boost::python::extract; | |
| using Kernel::IValidator; | |
| // Ensure GIL for all Boost.Python interactions in this scope | |
| GlobalInterpreterLock gil; | |
| std::unique_ptr<Kernel::Property> valueProp; | |
| if (isNone(validator)) { | |
| valueProp = std::make_unique<PythonObjectProperty>(name, defaultValue, direction); | |
| } else { | |
| const auto extractor = extract<IValidator *>(validator); | |
| if (!extractor.check() || extractor() == nullptr) { | |
| throw std::invalid_argument("PythonObjectTypeHandler: validator must be a Mantid Kernel::IValidator"); | |
| } | |
| const IValidator *propValidator = extractor(); | |
| valueProp = | |
| std::make_unique<PythonObjectProperty>(name, defaultValue, propValidator->clone(), direction); | |
| } | |
| return valueProp; | |
| } |
| def test_set_property_on_algorithm_PyObject(self): | ||
| fake = FakeAlgorithm() | ||
| fake.initialize() | ||
|
|
||
| value = 5 | ||
| fake.setProperty("PyObject", value) | ||
|
|
||
| # It is important that both the value and address match to prove it | ||
| # is just the same object that we got back | ||
| self.assertEqual(value, fake.getProperty("PyObject").value) | ||
| self.assertEqual(id(value), id(fake.getProperty("PyObject").value)) | ||
|
|
There was a problem hiding this comment.
Fix identity test: small-int interning makes this test vacuous.
5 is interned in CPython, so id(5) is always identical and doesn’t prove pass-through identity. Use a unique object and assertIs.
- value = 5
+ value = object()
fake.setProperty("PyObject", value)
- # It is important that both the value and address match to prove it
- # is just the same object that we got back
- self.assertEqual(value, fake.getProperty("PyObject").value)
- self.assertEqual(id(value), id(fake.getProperty("PyObject").value))
+ # Ensure the exact same Python object instance is returned
+ self.assertIs(value, fake.getProperty("PyObject").value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_set_property_on_algorithm_PyObject(self): | |
| fake = FakeAlgorithm() | |
| fake.initialize() | |
| value = 5 | |
| fake.setProperty("PyObject", value) | |
| # It is important that both the value and address match to prove it | |
| # is just the same object that we got back | |
| self.assertEqual(value, fake.getProperty("PyObject").value) | |
| self.assertEqual(id(value), id(fake.getProperty("PyObject").value)) | |
| def test_set_property_on_algorithm_PyObject(self): | |
| fake = FakeAlgorithm() | |
| fake.initialize() | |
| value = object() | |
| fake.setProperty("PyObject", value) | |
| # Ensure the exact same Python object instance is returned | |
| self.assertIs(value, fake.getProperty("PyObject").value) |
🤖 Prompt for AI Agents
In
Framework/PythonInterface/test/python/mantid/kernel/PythonObjectPropertyTest.py
around lines 55 to 66, the test uses the small integer 5 which is interned in
CPython so comparing ids is meaningless; replace the primitive with a unique
object (e.g. value = object() or value = SomeMutableUniqueObject()) and replace
the equality/id assertions with an identity assertion such as
self.assertIs(fake.getProperty("PyObject").value, value) to verify pass-through
identity.
|
I think it's the failing Windows test on d02fb87 that is the one to work on resolving. |
Description of work
Closes #xxxx.
To test:
Reviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
Gatekeeper
As per the gatekeeping guidelines: