-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for serializing/deserializing user-defined stream-level metadata. #133
Changes from all commits
a4f92a3
72c4213
f49a098
29d0394
d492645
35449be
4247452
e4344f0
eb63020
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,24 +3,29 @@ | |
#include "PyDeserializer.hpp" | ||
|
||
#include <new> | ||
#include <string> | ||
#include <system_error> | ||
#include <type_traits> | ||
#include <utility> | ||
|
||
#include <clp/ffi/ir_stream/decoding_methods.hpp> | ||
#include <clp/ffi/ir_stream/Deserializer.hpp> | ||
#include <clp/ffi/ir_stream/IrUnitType.hpp> | ||
#include <clp/ffi/ir_stream/protocol_constants.hpp> | ||
#include <clp/ffi/KeyValuePairLogEvent.hpp> | ||
#include <clp/ffi/SchemaTree.hpp> | ||
#include <clp/time_types.hpp> | ||
#include <clp/TraceableException.hpp> | ||
#include <json/single_include/nlohmann/json.hpp> | ||
|
||
#include <clp_ffi_py/api_decoration.hpp> | ||
#include <clp_ffi_py/error_messages.hpp> | ||
#include <clp_ffi_py/ir/native/DeserializerBufferReader.hpp> | ||
#include <clp_ffi_py/ir/native/error_messages.hpp> | ||
#include <clp_ffi_py/ir/native/PyKeyValuePairLogEvent.hpp> | ||
#include <clp_ffi_py/Py_utils.hpp> | ||
#include <clp_ffi_py/PyObjectCast.hpp> | ||
#include <clp_ffi_py/PyObjectUtils.hpp> | ||
#include <clp_ffi_py/utils.hpp> | ||
|
||
namespace clp_ffi_py::ir::native { | ||
|
@@ -67,6 +72,23 @@ PyDoc_STRVAR( | |
); | ||
CLP_FFI_PY_METHOD auto PyDeserializer_deserialize_log_event(PyDeserializer* self) -> PyObject*; | ||
|
||
/** | ||
* Callback of `PyDeserializer`'s `get_user_defined_metadata`. | ||
*/ | ||
PyDoc_STRVAR( | ||
cPyDeserializerGetUserDefinedMetadataDoc, | ||
"get_user_defined_metadata(self)\n" | ||
"--\n\n" | ||
"Gets the user-defined stream-level metadata.\n\n" | ||
":return:\n" | ||
" - The deserialized user-defined stream-level metadata, loaded as a" | ||
" dictionary.\n" | ||
" - None if user-defined stream-level metadata was not given in the deserialized" | ||
" IR stream.\n" | ||
":rtype: dict | None\n" | ||
); | ||
CLP_FFI_PY_METHOD auto PyDeserializer_get_user_defined_metadata(PyDeserializer* self) -> PyObject*; | ||
|
||
/** | ||
* Callback of `PyDeserializer`'s deallocator. | ||
*/ | ||
|
@@ -79,6 +101,11 @@ PyMethodDef PyDeserializer_method_table[]{ | |
METH_NOARGS, | ||
static_cast<char const*>(cPyDeserializerDeserializeLogEventDoc)}, | ||
|
||
{"get_user_defined_metadata", | ||
py_c_function_cast(PyDeserializer_get_user_defined_metadata), | ||
METH_NOARGS, | ||
static_cast<char const*>(cPyDeserializerGetUserDefinedMetadataDoc)}, | ||
|
||
{nullptr} | ||
}; | ||
|
||
|
@@ -153,6 +180,40 @@ CLP_FFI_PY_METHOD auto PyDeserializer_deserialize_log_event(PyDeserializer* self | |
return self->deserialize_log_event(); | ||
} | ||
|
||
CLP_FFI_PY_METHOD auto PyDeserializer_get_user_defined_metadata(PyDeserializer* self) -> PyObject* { | ||
auto const* user_defined_metadata{self->get_user_defined_metadata()}; | ||
if (nullptr == user_defined_metadata) { | ||
Py_RETURN_NONE; | ||
} | ||
|
||
std::string json_str; | ||
try { | ||
json_str = user_defined_metadata->dump(); | ||
} catch (nlohmann::json::exception const& ex) { | ||
PyErr_Format( | ||
PyExc_RuntimeError, | ||
"Failed to serialize the user-defined stream-level metadata into a JSON string." | ||
" Error: %s", | ||
ex.what() | ||
); | ||
return nullptr; | ||
} | ||
|
||
PyObjectPtr<PyObject> py_metadata_dict{py_utils_parse_json_str(json_str)}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry for not being able find any previous discussions on this - we have considered the https://github.com/pybind/pybind11_json and other alternatives discussed at https://www.github.com/nlohmann/json/issues/1014 , right? just curious, for more efficient conversions, do we have plans to make use of those or create our own bindings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this isn't in any of the critical path anyways, I would probably defer it to future release |
||
if (nullptr == py_metadata_dict) { | ||
return nullptr; | ||
} | ||
if (false == static_cast<bool>(PyDict_Check(py_metadata_dict.get()))) { | ||
PyErr_SetString( | ||
PyExc_TypeError, | ||
"Failed to convert the user-defined stream-level metadata into a dictionary." | ||
); | ||
return nullptr; | ||
} | ||
|
||
return py_metadata_dict.release(); | ||
} | ||
|
||
CLP_FFI_PY_METHOD auto PyDeserializer_dealloc(PyDeserializer* self) -> void { | ||
self->clean(); | ||
Py_TYPE(self)->tp_free(py_reinterpret_cast<PyObject>(self)); | ||
|
@@ -282,6 +343,17 @@ auto PyDeserializer::deserialize_log_event() -> PyObject* { | |
Py_RETURN_NONE; | ||
} | ||
|
||
auto PyDeserializer::get_user_defined_metadata() const -> nlohmann::json const* { | ||
auto const& metadata{m_deserializer->get_metadata()}; | ||
std::string const user_defined_metadata_key{ | ||
clp::ffi::ir_stream::cProtocol::Metadata::UserDefinedMetadataKey | ||
}; | ||
if (false == metadata.contains(user_defined_metadata_key)) { | ||
return nullptr; | ||
} | ||
return &metadata.at(user_defined_metadata_key); | ||
} | ||
|
||
auto PyDeserializer::handle_log_event(clp::ffi::KeyValuePairLogEvent&& log_event) -> IRErrorCode { | ||
if (has_unreleased_deserialized_log_event()) { | ||
// This situation may occur if the deserializer methods return an error after the last | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be less handling for API users if we return an empty dictionary if "user-defined stream-level metadata was not given in the deserialized IR stream"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, it does seem in the Serializer we do permit users to provide a
None
or "{}" value. I haven't traced down to the CLP code but I think the given metadata string will be encoded as-is?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to return
None
indicating that the user-defined metadata wasn't specified in the source.It is possible to encode an empty dictionary as the metadata though.