From 245ffafaf9c4d4f26935e5f997c92e29e920b87e Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Thu, 4 Jun 2026 16:50:03 -0700 Subject: [PATCH 01/15] Address a lifespan issue for the adapter parameters --- onnxruntime/python/onnxruntime_pybind_lora.cc | 34 +++++++++++++----- .../test/python/onnxruntime_test_python.py | 35 +++++++++++++++++++ 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index af8365418e5ea..e4c1d5305da59 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -40,7 +40,7 @@ struct PyAdapterFormatReaderWriter { : format_version_(format_version), adapter_version_(adapter_version), model_version_(model_version), - loaded_adater_(std::move(loaded_adapter)), + loaded_adapter_(std::move(loaded_adapter)), parameters_(std::move(params)) {} int format_version_{adapters::kAdapterFormatVersion}; @@ -48,7 +48,7 @@ struct PyAdapterFormatReaderWriter { int model_version_{0}; // This container is used when reading the the file so // OrtValue objects can be backed by it. Not exposed to Python - std::optional loaded_adater_; + std::optional loaded_adapter_; // This is a dictionary of string -> OrtValue // this is populated directly on write and // built on top of the loaded_adapter on read @@ -65,11 +65,6 @@ void addAdapterFormatMethods(pybind11::module& m) { "format_version", [](const PyAdapterFormatReaderWriter* reader_writer) -> int { return reader_writer->format_version_; }, R"pbdoc("Enables user to read format version stored in the file")pbdoc") - .def_property( - "adapter_version", - [](const PyAdapterFormatReaderWriter* reader_writer) -> int { return reader_writer->adapter_version_; }, - [](PyAdapterFormatReaderWriter* reader_writer, int adapter_version) -> void { reader_writer->adapter_version_ = adapter_version; }, - R"pbdoc("Enables user to read format version stored in the file")pbdoc") .def_property( "adapter_version", [](const PyAdapterFormatReaderWriter* reader_writer) -> int { return reader_writer->adapter_version_; }, @@ -82,11 +77,34 @@ void addAdapterFormatMethods(pybind11::module& m) { R"pbdoc("Enables user to read/write model version this adapter was created for")pbdoc") .def_property( "parameters", + // The dict's values are pybind11 wrappers around raw OrtValue* pointers + // that alias storage owned by PyAdapterFormatReaderWriter::loaded_adapter_ + // (see read_adapter below: entries are produced via py::cast(&ort_value), + // i.e. non-owning views into the LoraAdapter's params_values_ map). + // + // Without an explicit keep-alive, Python users following the natural + // pattern + // + // params = ort.AdapterFormat.read_adapter(path).parameters + // + // would silently get a dict of dangling pointers: the temporary + // AdapterFormat (and with it the LoraAdapter that backs every OrtValue) + // is destroyed at the end of the expression, and any subsequent access + // such as params["x"].numpy() would dereference freed memory. + // + // Python callers reasonably expect refcounting to keep parents alive + // while derived objects are still referenced; this is the binding + // author's responsibility for non-owning views. py::keep_alive<0, 1> + // ties the returned dict (return value, index 0) to the owning + // PyAdapterFormatReaderWriter (self, index 1), so the parent — and + // therefore the underlying adapter buffer — survives at least as long + // as the dict the caller holds. [](const PyAdapterFormatReaderWriter* reader_writer) -> py::dict { return reader_writer->parameters_; }, [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { reader_writer->parameters_ = parameters; }, - R"pbdoc("Enables user to read/write adapter version stored in the file")pbdoc") + py::keep_alive<0, 1>(), + R"pbdoc("Enables user to read/write the dictionary of adapter parameters (name -> OrtValue)")pbdoc") .def( "export_adapter", [](const PyAdapterFormatReaderWriter* reader_writer, const std::wstring& path) { diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 1368f6d53deb7..3dfbc48c240ad 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2103,6 +2103,41 @@ def test_adater_export_read(self): self.assertEqual(expected_val.shape(), value.shape()) np.testing.assert_allclose(value.numpy(), expected_val.numpy()) + def test_adapter_parameters_keep_alive(self): + # Regression test: AdapterFormat.parameters returned a dict of OrtValue + # views into the parent adapter without a pybind11 keep_alive policy. + # The natural pattern below dropped the parent and left the dict with + # dangling pointers, causing a use-after-free on the next access. + adapter_version = 1 + model_version = 1 + file_path = pathlib.Path(os.path.realpath(__file__)).parent + file_path = str(file_path / "test_adapter_keep_alive.onnx_adapter") + + float_data_type = 1 + val = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] + param_1 = np.array(val).astype(np.float32).reshape(5, 2) + + ort_val_1 = onnxrt.OrtValue.ortvalue_from_numpy_with_onnx_type(param_1, float_data_type) + + adapter_format = onnxrt.AdapterFormat() + adapter_format.set_adapter_version(adapter_version) + adapter_format.set_model_version(model_version) + adapter_format.set_parameters({"param_1": ort_val_1}) + adapter_format.export_adapter(file_path) + + try: + # Drop the AdapterFormat temporary; only `params` keeps a reference. + params = onnxrt.AdapterFormat.read_adapter(file_path).parameters + gc.collect() + + self.assertIn("param_1", params) + value = params["param_1"] + self.assertTrue(value.is_tensor()) + self.assertEqual(value.shape(), [5, 2]) + np.testing.assert_allclose(value.numpy(), param_1) + finally: + os.remove(file_path) + def test_run_with_adapter(self): model_path = get_name("lora/two_params_lora_model.onnx") file_path = os.getcwd() + "/" + get_name("lora/two_params_lora_model.onnx_adapter") From 1fcdbeee8f98d481d838f477e5fd201c85e5e232 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Thu, 4 Jun 2026 17:01:38 -0700 Subject: [PATCH 02/15] Reject string tensors in AdapterFormat.export_adapter export_adapter wrote tensor.DataRaw() for tensor.SizeInBytes() bytes regardless of element type. For tensor(string) parameters this copied the std::string object representation - heap pointers and SSO padding - directly into Parameter.raw_data, leaking runtime addresses (ASLR bypass) and uninitialized bytes, and producing an adapter that cannot be safely loaded (reinterpreting the saved bytes as std::string objects is undefined behavior). Reject STRING element type with a clear error and defer opening the output file until after validation/serialization so a rejected export does not leave a stray empty file behind. Test: test_adapter_export_rejects_string_tensors asserts export_adapter raises on tensor(string) parameters and leaves no file on disk. --- onnxruntime/python/onnxruntime_pybind_lora.cc | 26 +++++++++++++---- .../test/python/onnxruntime_test_python.py | 28 +++++++++++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index e4c1d5305da59..651ed53f4599b 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -109,24 +109,40 @@ void addAdapterFormatMethods(pybind11::module& m) { "export_adapter", [](const PyAdapterFormatReaderWriter* reader_writer, const std::wstring& path) { std::filesystem::path file_path(path); - std::ofstream file(file_path, std::ios::binary); - if (file.fail()) { - ORT_THROW("Failed to open file:", file_path, " for writing."); - } adapters::utils::AdapterFormatBuilder format_builder; for (auto& [n, value] : reader_writer->parameters_) { const std::string param_name = py::str(n); const OrtValue* ort_value = value.cast(); const Tensor& tensor = ort_value->Get(); + const auto element_type = tensor.GetElementType(); + // Reject string tensors: Tensor::DataRaw() for a string tensor points to an + // array of std::string objects, and SizeInBytes() counts the sizeof(std::string) + // object representation (which contains heap pointers and uninitialized + // padding). Serializing those bytes would (a) leak runtime addresses + // (defeating ASLR) and uninitialized heap memory into the adapter file, + // and (b) produce an unloadable adapter, since reading the bytes back as + // std::string objects is undefined behavior. The adapter format has no + // representation for string tensors. + if (element_type == ONNX_NAMESPACE::TensorProto_DataType_STRING) { + ORT_THROW("Lora adapter parameter '", param_name, + "' has element type STRING, which is not supported by the adapter format."); + } const auto data_span = gsl::make_span(reinterpret_cast(tensor.DataRaw()), tensor.SizeInBytes()); format_builder.AddParameter( - param_name, static_cast(tensor.GetElementType()), + param_name, static_cast(element_type), tensor.Shape().GetDims(), data_span); } + // Only open the output file after all parameters have been validated and + // serialized in memory, so a rejected export does not leave a stray empty file. + std::ofstream file(file_path, std::ios::binary); + if (file.fail()) { + ORT_THROW("Failed to open file:", file_path, " for writing."); + } + auto format_span = format_builder.FinishWithSpan(reader_writer->adapter_version_, reader_writer->model_version_); if (file.write(reinterpret_cast(format_span.data()), format_span.size()).fail()) { diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 3dfbc48c240ad..024641ed55d91 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2138,6 +2138,34 @@ def test_adapter_parameters_keep_alive(self): finally: os.remove(file_path) + def test_adapter_export_rejects_string_tensors(self): + # Regression test: export_adapter previously serialized Tensor::DataRaw() + # for SizeInBytes() bytes regardless of element type. For string tensors + # that copied the std::string object representation (heap pointers and + # uninitialized padding) into the adapter file, leaking runtime addresses + # (ASLR bypass) and producing an unloadable adapter. Export must reject + # string-typed parameters. + string_data_type = 8 # ONNX TensorProto.STRING + string_array = np.array(["hello", "world"], dtype=object) + ort_val_str = onnxrt.OrtValue.ortvalue_from_numpy_with_onnx_type(string_array, string_data_type) + + adapter_format = onnxrt.AdapterFormat() + adapter_format.set_adapter_version(1) + adapter_format.set_model_version(1) + adapter_format.set_parameters({"str_param": ort_val_str}) + + file_path = pathlib.Path(os.path.realpath(__file__)).parent + file_path = str(file_path / "test_adapter_string_reject.onnx_adapter") + + try: + with self.assertRaises(Exception) as ctx: + adapter_format.export_adapter(file_path) + self.assertIn("STRING", str(ctx.exception)) + self.assertFalse(os.path.exists(file_path), "adapter file must not be created when export is rejected") + finally: + if os.path.exists(file_path): + os.remove(file_path) + def test_run_with_adapter(self): model_path = get_name("lora/two_params_lora_model.onnx") file_path = os.getcwd() + "/" + get_name("lora/two_params_lora_model.onnx_adapter") From 9d58ed729b98ef5601284ff34b7ba84e77f9aa3a Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Thu, 4 Jun 2026 18:42:12 -0700 Subject: [PATCH 03/15] Make sure Load() has strong exception safety --- onnxruntime/core/session/lora_adapters.cc | 37 +++++++++++++++++------ onnxruntime/core/session/lora_adapters.h | 8 ++++- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/onnxruntime/core/session/lora_adapters.cc b/onnxruntime/core/session/lora_adapters.cc index de3acb22e12f8..0f25dcd85355d 100644 --- a/onnxruntime/core/session/lora_adapters.cc +++ b/onnxruntime/core/session/lora_adapters.cc @@ -37,17 +37,35 @@ void LoraAdapter::Load(const std::filesystem::path& file_path) { } void LoraAdapter::Load(std::vector buffer) { - adapter_ = adapters::utils::ValidateAndGetAdapterFromBytes(buffer); + // Build everything that can throw against locals first, so a malformed adapter + // leaves *this unchanged (strong exception guarantee). Only after the new + // parameter map has been fully constructed do we release the previous backing + // storage and swap in the new state. ValidateAndGetAdapterFromBytes returns a + // pointer that aliases `buffer`'s data; std::vector's move preserves the data + // pointer, so the alias remains valid after we move `buffer` into buffer_. + const auto* new_adapter = adapters::utils::ValidateAndGetAdapterFromBytes(buffer); + auto new_params_values = BuildParamsValues(new_adapter); + + // Commit. None of the operations below can throw. buffer_.emplace(std::move(buffer)); - InitializeParamsValues(); + adapter_ = new_adapter; + params_values_ = std::move(new_params_values); } void LoraAdapter::MemoryMap(const std::filesystem::path& file_path) { auto [mapped_memory, file_size] = adapters::utils::MemoryMapAdapterFile(file_path); auto u8_span = ReinterpretAsSpan(gsl::make_span(mapped_memory.get(), file_size)); - adapter_ = adapters::utils::ValidateAndGetAdapterFromBytes(u8_span); + + // Build everything that can throw against locals first; see Load() above. + // Moving Env::MappedMemoryPtr preserves the underlying pointer, so OrtValues + // built against `mapped_memory.get()` remain valid after the move into buffer_. + const auto* new_adapter = adapters::utils::ValidateAndGetAdapterFromBytes(u8_span); + auto new_params_values = BuildParamsValues(new_adapter); + + // Commit. None of the operations below can throw. buffer_.emplace(std::move(mapped_memory), file_size); - InitializeParamsValues(); + adapter_ = new_adapter; + params_values_ = std::move(new_params_values); } static std::unique_ptr GetDataTransfer(const OrtMemoryInfo& mem_info) { @@ -82,10 +100,9 @@ static Status CreateOrtValueOnDevice(const OrtValue& ort_value_mapped, return Status::OK(); } -void LoraAdapter::InitializeParamsValues() { - if (adapter_ == nullptr) { - ORT_THROW("Adapter is not loaded yet."); - } +std::unordered_map +LoraAdapter::BuildParamsValues(const adapters::Adapter* adapter) const { + ORT_ENFORCE(adapter != nullptr, "Adapter is not loaded yet."); std::unique_ptr data_transfer; if (device_allocator_) { @@ -95,7 +112,7 @@ void LoraAdapter::InitializeParamsValues() { } } - const auto* params = adapter_->parameters(); + const auto* params = adapter->parameters(); ORT_ENFORCE(params != nullptr, "Params absent"); std::unordered_map params_values; params_values.reserve(params->size()); @@ -117,7 +134,7 @@ void LoraAdapter::InitializeParamsValues() { } } - params_values_.swap(params_values); + return params_values; } } // namespace lora diff --git a/onnxruntime/core/session/lora_adapters.h b/onnxruntime/core/session/lora_adapters.h index 77534b2bb7d15..7610779b2d69c 100644 --- a/onnxruntime/core/session/lora_adapters.h +++ b/onnxruntime/core/session/lora_adapters.h @@ -151,7 +151,13 @@ class LoraAdapter { } private: - void InitializeParamsValues(); + // Build a fresh parameter map from the supplied adapter pointer (which must alias + // memory the caller is keeping alive). Throws on validation failure. Does not + // touch this->params_values_ / buffer_ / adapter_; callers are responsible for + // committing the result only after they have also committed the new backing + // storage. Keeping this function free of side effects on *this is what gives + // Load()/MemoryMap() the strong exception guarantee. + std::unordered_map BuildParamsValues(const adapters::Adapter* adapter) const; struct BufferHolder { explicit BufferHolder(std::vector buffer) : buffer_(std::move(buffer)) {} From 5f71cc12cfdd063bb0b62451b9f9cf3345ee351e Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Fri, 5 Jun 2026 13:13:04 -0700 Subject: [PATCH 04/15] Fix def_property keep_alive and FinishWithSpan ordering pybind11's def_property() does not accept keep_alive directly (static_assert fires). Wrap the getter in py::cpp_function so the policy can be attached, restoring the keep-alive behavior intended by 245ffaf. Also compute the serialized adapter span before opening the output file, per PR review: a failure inside FinishWithSpan should not leave a stray empty file behind. --- onnxruntime/python/onnxruntime_pybind_lora.cc | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index 651ed53f4599b..c2ca36d62009a 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -98,12 +98,16 @@ void addAdapterFormatMethods(pybind11::module& m) { // ties the returned dict (return value, index 0) to the owning // PyAdapterFormatReaderWriter (self, index 1), so the parent — and // therefore the underlying adapter buffer — survives at least as long - // as the dict the caller holds. - [](const PyAdapterFormatReaderWriter* reader_writer) -> py::dict { return reader_writer->parameters_; }, - [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { - reader_writer->parameters_ = parameters; - }, - py::keep_alive<0, 1>(), + // as the dict the caller holds. def_property() does not accept + // keep_alive directly, so the policy is attached to the getter via + // py::cpp_function. + py::cpp_function( + [](const PyAdapterFormatReaderWriter* reader_writer) -> py::dict { return reader_writer->parameters_; }, + py::keep_alive<0, 1>()), + py::cpp_function( + [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { + reader_writer->parameters_ = parameters; + }), R"pbdoc("Enables user to read/write the dictionary of adapter parameters (name -> OrtValue)")pbdoc") .def( "export_adapter", @@ -136,15 +140,18 @@ void addAdapterFormatMethods(pybind11::module& m) { tensor.Shape().GetDims(), data_span); } - // Only open the output file after all parameters have been validated and - // serialized in memory, so a rejected export does not leave a stray empty file. + // Build the entire adapter image in memory before touching the + // filesystem, so any failure (string-tensor rejection above, builder + // errors below, allocation failure inside FinishWithSpan, etc.) + // leaves no stray file behind. + auto format_span = format_builder.FinishWithSpan(reader_writer->adapter_version_, + reader_writer->model_version_); + std::ofstream file(file_path, std::ios::binary); if (file.fail()) { ORT_THROW("Failed to open file:", file_path, " for writing."); } - auto format_span = format_builder.FinishWithSpan(reader_writer->adapter_version_, - reader_writer->model_version_); if (file.write(reinterpret_cast(format_span.data()), format_span.size()).fail()) { ORT_THROW("Failed to write :", std::to_string(format_span.size()), " bytes to ", file_path); } From d2530192f6068f027e7eccb45051cd53e05a7ee1 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Fri, 5 Jun 2026 13:46:29 -0700 Subject: [PATCH 05/15] Propagate adapter keep-alive through Python AdapterFormat wrapper The C-level pybind11 keep_alive on parameters tied the returned dict to the C AdapterFormat, but AdapterFormat.get_parameters() in the Python wrapper rebuilt a fresh dict of OrtValue wrappers and dropped the C dict, losing the link. The user-facing pattern params = ort.AdapterFormat.read_adapter(p).get_parameters() would still leave each OrtValue dangling once the temporary AdapterFormat was destroyed. Pin the C AdapterFormat on every OrtValue handed back, so callers who keep any of them keep the backing adapter too. Also fix the keep-alive regression test, which used a non-existent .parameters attribute on the Python wrapper (would have failed with AttributeError before exercising the UAF), and add a stronger assertion that an individual OrtValue stays valid after the dict is dropped. --- .../onnxruntime_inference_collection.py | 18 ++++++++++++++- .../test/python/onnxruntime_test_python.py | 22 ++++++++++++++----- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/onnxruntime/python/onnxruntime_inference_collection.py b/onnxruntime/python/onnxruntime_inference_collection.py index 59ddfc49d71a6..b164aae45367a 100644 --- a/onnxruntime/python/onnxruntime_inference_collection.py +++ b/onnxruntime/python/onnxruntime_inference_collection.py @@ -107,7 +107,23 @@ def set_parameters(self, params: dict[str, OrtValue]) -> None: self._adapter.parameters = {k: v._ortvalue for k, v in params.items()} def get_parameters(self) -> dict[str, OrtValue]: - return {k: OrtValue(v) for k, v in self._adapter.parameters.items()} + # The C-level OrtValues stored in `self._adapter.parameters` are non-owning + # views into the adapter's internal storage. The C-level `parameters` + # getter has a pybind11 keep_alive policy that ties the returned dict to + # the C AdapterFormat, but here we build a fresh Python dict of new + # `OrtValue` wrappers; that fresh dict has no link back to self._adapter, + # so without an explicit reference here, users who do + # params = ort.AdapterFormat.read_adapter(path).get_parameters() + # would get OrtValues that dangle as soon as the temporary AdapterFormat + # is destroyed. Pin self._adapter on every returned OrtValue so the + # backing adapter stays alive at least as long as any value held by the + # caller. + result: dict[str, OrtValue] = {} + for k, v in self._adapter.parameters.items(): + ov = OrtValue(v) + ov._adapter_keepalive = self._adapter + result[k] = ov + return result def check_and_normalize_provider_args( diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 024641ed55d91..3b78c7fed282e 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2104,10 +2104,15 @@ def test_adater_export_read(self): np.testing.assert_allclose(value.numpy(), expected_val.numpy()) def test_adapter_parameters_keep_alive(self): - # Regression test: AdapterFormat.parameters returned a dict of OrtValue - # views into the parent adapter without a pybind11 keep_alive policy. - # The natural pattern below dropped the parent and left the dict with - # dangling pointers, causing a use-after-free on the next access. + # Regression test: AdapterFormat.get_parameters() returned OrtValue + # wrappers over non-owning C OrtValue* views into the parent adapter, + # with nothing keeping the parent alive. The natural pattern below + # dropped the parent and left the dict with dangling pointers, causing + # a use-after-free on the next access. Both layers must keep the + # backing adapter alive: the C-level `parameters` getter via a pybind11 + # keep_alive policy on the returned dict, and the Python wrapper by + # pinning the adapter on every OrtValue it hands back from + # get_parameters(). adapter_version = 1 model_version = 1 file_path = pathlib.Path(os.path.realpath(__file__)).parent @@ -2127,7 +2132,7 @@ def test_adapter_parameters_keep_alive(self): try: # Drop the AdapterFormat temporary; only `params` keeps a reference. - params = onnxrt.AdapterFormat.read_adapter(file_path).parameters + params = onnxrt.AdapterFormat.read_adapter(file_path).get_parameters() gc.collect() self.assertIn("param_1", params) @@ -2135,6 +2140,13 @@ def test_adapter_parameters_keep_alive(self): self.assertTrue(value.is_tensor()) self.assertEqual(value.shape(), [5, 2]) np.testing.assert_allclose(value.numpy(), param_1) + + # Also drop the dict; an individual OrtValue must keep the adapter + # alive on its own (we pin it on every value in get_parameters()). + single_value = params["param_1"] + del params + gc.collect() + np.testing.assert_allclose(single_value.numpy(), param_1) finally: os.remove(file_path) From 22cf53c9820ea015ed6ca7dd34cdb7264d998020 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Fri, 5 Jun 2026 15:32:43 -0700 Subject: [PATCH 06/15] Fix string-tensor regression test: construct OrtValue via Constant model ortvalue_from_numpy_with_onnx_type rejected the string numpy array (it validates numpy element size against the std::string object size), so the test errored out before reaching export_adapter. There is no public Python API that constructs a string-typed OrtValue directly; the only working path is to obtain one as a session output. Build a tiny in-memory Constant model whose output is a string tensor and use that OrtValue to drive the export rejection check. --- .../test/python/onnxruntime_test_python.py | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 3b78c7fed282e..9a749571a7f60 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2157,9 +2157,28 @@ def test_adapter_export_rejects_string_tensors(self): # uninitialized padding) into the adapter file, leaking runtime addresses # (ASLR bypass) and producing an unloadable adapter. Export must reject # string-typed parameters. - string_data_type = 8 # ONNX TensorProto.STRING - string_array = np.array(["hello", "world"], dtype=object) - ort_val_str = onnxrt.OrtValue.ortvalue_from_numpy_with_onnx_type(string_array, string_data_type) + # + # There is no public Python API to construct a string-typed OrtValue + # directly (ortvalue_from_numpy rejects non-numeric arrays), so we + # obtain one by running a tiny Constant model whose only output is a + # string tensor. + from onnx import TensorProto, helper # noqa: PLC0415 + + const_node = helper.make_node( + "Constant", + inputs=[], + outputs=["str_out"], + value=helper.make_tensor("v", TensorProto.STRING, dims=[2], vals=[b"hello", b"world"]), + ) + graph = helper.make_graph( + [const_node], + "string_const", + inputs=[], + outputs=[helper.make_tensor_value_info("str_out", TensorProto.STRING, [2])], + ) + model = helper.make_model(graph, opset_imports=[helper.make_opsetid("", 17)]) + sess = onnxrt.InferenceSession(model.SerializeToString(), providers=onnxrt.get_available_providers()) + ort_val_str = sess.run_with_ort_values(["str_out"], {})[0] adapter_format = onnxrt.AdapterFormat() adapter_format.set_adapter_version(1) From d410c8121a4563cd4330923c453453ca88391cdc Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Fri, 5 Jun 2026 15:54:40 -0700 Subject: [PATCH 07/15] Drop unworkable C-level dict keep_alive; rely on Python wrapper py::keep_alive<0,1> on a returned py::dict raises 'cannot create weak reference to dict' at runtime: pybind11 keep_alive takes a weakref to the patient (the returned dict), and Python dicts are not weak-referenceable. The C-level policy was therefore broken in two ways across the two pybind11 versions seen so far: a static_assert in the newer pybind11 (def_property does not accept keep_alive) and a runtime TypeError once routed through cpp_function. Lifetime safety for the documented user-facing pattern params = ort.AdapterFormat.read_adapter(p).get_parameters() is provided by the Python wrapper AdapterFormat.get_parameters, which already pins the owning C AdapterFormat on every OrtValue it hands back. Document this in the C-level binding and remove the broken keep_alive. --- onnxruntime/python/onnxruntime_pybind_lora.cc | 41 ++++++++----------- .../test/python/onnxruntime_test_python.py | 8 ++-- 2 files changed, 20 insertions(+), 29 deletions(-) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index c2ca36d62009a..421e02265feb2 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -82,32 +82,25 @@ void addAdapterFormatMethods(pybind11::module& m) { // (see read_adapter below: entries are produced via py::cast(&ort_value), // i.e. non-owning views into the LoraAdapter's params_values_ map). // - // Without an explicit keep-alive, Python users following the natural - // pattern + // We cannot attach py::keep_alive<0, 1> here: pybind11 keep_alive + // takes a weak reference to the returned object as the "patient", and + // Python `dict` does not support weak references, so the policy + // raises TypeError at call time. Lifetime safety for the documented + // user-facing pattern // - // params = ort.AdapterFormat.read_adapter(path).parameters + // params = ort.AdapterFormat.read_adapter(path).get_parameters() // - // would silently get a dict of dangling pointers: the temporary - // AdapterFormat (and with it the LoraAdapter that backs every OrtValue) - // is destroyed at the end of the expression, and any subsequent access - // such as params["x"].numpy() would dereference freed memory. - // - // Python callers reasonably expect refcounting to keep parents alive - // while derived objects are still referenced; this is the binding - // author's responsibility for non-owning views. py::keep_alive<0, 1> - // ties the returned dict (return value, index 0) to the owning - // PyAdapterFormatReaderWriter (self, index 1), so the parent — and - // therefore the underlying adapter buffer — survives at least as long - // as the dict the caller holds. def_property() does not accept - // keep_alive directly, so the policy is attached to the getter via - // py::cpp_function. - py::cpp_function( - [](const PyAdapterFormatReaderWriter* reader_writer) -> py::dict { return reader_writer->parameters_; }, - py::keep_alive<0, 1>()), - py::cpp_function( - [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { - reader_writer->parameters_ = parameters; - }), + // is therefore provided by the Python wrapper + // (onnxruntime_inference_collection.AdapterFormat.get_parameters), + // which pins the owning C AdapterFormat on every returned OrtValue. + // This raw pybind11 class is an implementation detail; callers that + // bypass the wrapper and reach for `C.AdapterFormat(...).parameters` + // directly are responsible for keeping the AdapterFormat alive while + // they use the dict. + [](const PyAdapterFormatReaderWriter* reader_writer) -> py::dict { return reader_writer->parameters_; }, + [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { + reader_writer->parameters_ = parameters; + }, R"pbdoc("Enables user to read/write the dictionary of adapter parameters (name -> OrtValue)")pbdoc") .def( "export_adapter", diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 9a749571a7f60..f53f674a012ea 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2108,11 +2108,9 @@ def test_adapter_parameters_keep_alive(self): # wrappers over non-owning C OrtValue* views into the parent adapter, # with nothing keeping the parent alive. The natural pattern below # dropped the parent and left the dict with dangling pointers, causing - # a use-after-free on the next access. Both layers must keep the - # backing adapter alive: the C-level `parameters` getter via a pybind11 - # keep_alive policy on the returned dict, and the Python wrapper by - # pinning the adapter on every OrtValue it hands back from - # get_parameters(). + # a use-after-free on the next access. The Python wrapper now pins the + # owning C AdapterFormat on every OrtValue it hands back, so callers + # that keep any of the values keep the backing adapter alive too. adapter_version = 1 model_version = 1 file_path = pathlib.Path(os.path.realpath(__file__)).parent From fab4390c5f2921e4f8704ab70d79ae8f0f35b9a7 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Sun, 7 Jun 2026 11:28:31 -0700 Subject: [PATCH 08/15] Address pybind life time concerns --- .../onnxruntime_inference_collection.py | 22 +-- onnxruntime/python/onnxruntime_pybind_lora.cc | 129 ++++++++++++------ .../test/python/onnxruntime_test_python.py | 16 ++- 3 files changed, 98 insertions(+), 69 deletions(-) diff --git a/onnxruntime/python/onnxruntime_inference_collection.py b/onnxruntime/python/onnxruntime_inference_collection.py index b164aae45367a..5319d5d473d6b 100644 --- a/onnxruntime/python/onnxruntime_inference_collection.py +++ b/onnxruntime/python/onnxruntime_inference_collection.py @@ -107,23 +107,11 @@ def set_parameters(self, params: dict[str, OrtValue]) -> None: self._adapter.parameters = {k: v._ortvalue for k, v in params.items()} def get_parameters(self) -> dict[str, OrtValue]: - # The C-level OrtValues stored in `self._adapter.parameters` are non-owning - # views into the adapter's internal storage. The C-level `parameters` - # getter has a pybind11 keep_alive policy that ties the returned dict to - # the C AdapterFormat, but here we build a fresh Python dict of new - # `OrtValue` wrappers; that fresh dict has no link back to self._adapter, - # so without an explicit reference here, users who do - # params = ort.AdapterFormat.read_adapter(path).get_parameters() - # would get OrtValues that dangle as soon as the temporary AdapterFormat - # is destroyed. Pin self._adapter on every returned OrtValue so the - # backing adapter stays alive at least as long as any value held by the - # caller. - result: dict[str, OrtValue] = {} - for k, v in self._adapter.parameters.items(): - ov = OrtValue(v) - ov._adapter_keepalive = self._adapter - result[k] = ov - return result + # Each call to the C getter rebuilds the dict and pins the owning + # C AdapterFormat onto every returned OrtValue (pybind11 add_patient + # via keep_alive_impl), so callers can drop this AdapterFormat and + # the dict (or any individual value) keeps the backing adapter alive. + return {k: OrtValue(v) for k, v in self._adapter.parameters.items()} def check_and_normalize_provider_args( diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index 421e02265feb2..b4e876a3dafec 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -35,23 +35,25 @@ struct PyAdapterFormatReaderWriter { PyAdapterFormatReaderWriter() = default; PyAdapterFormatReaderWriter(int format_version, int adapter_version, int model_version, - lora::LoraAdapter&& loaded_adapter, - py::dict&& params) + lora::LoraAdapter&& loaded_adapter) : format_version_(format_version), adapter_version_(adapter_version), model_version_(model_version), - loaded_adapter_(std::move(loaded_adapter)), - parameters_(std::move(params)) {} + loaded_adapter_(std::move(loaded_adapter)) {} int format_version_{adapters::kAdapterFormatVersion}; int adapter_version_{0}; int model_version_{0}; - // This container is used when reading the the file so - // OrtValue objects can be backed by it. Not exposed to Python + // Populated only on the read path (read_adapter). When present, the + // `parameters` getter builds OrtValue views over its params_values_ map + // and pins this object as the keep_alive patient on each value. std::optional loaded_adapter_; - // This is a dictionary of string -> OrtValue - // this is populated directly on write and - // built on top of the loaded_adapter on read + // Populated only on the write path (user-supplied via the `parameters` + // setter) and consumed by export_adapter. On the read path this stays + // empty; the getter builds a fresh dict from loaded_adapter_ each call, + // which avoids an AdapterFormat -> dict -> OrtValue -> AdapterFormat + // reference cycle that pybind11's patient list would not let the GC + // break. py::dict parameters_; }; @@ -77,27 +79,55 @@ void addAdapterFormatMethods(pybind11::module& m) { R"pbdoc("Enables user to read/write model version this adapter was created for")pbdoc") .def_property( "parameters", - // The dict's values are pybind11 wrappers around raw OrtValue* pointers - // that alias storage owned by PyAdapterFormatReaderWriter::loaded_adapter_ - // (see read_adapter below: entries are produced via py::cast(&ort_value), - // i.e. non-owning views into the LoraAdapter's params_values_ map). + // Getter: build the dict on demand. On the read path each + // returned OrtValue is a non-owning view over storage owned by + // loaded_adapter_, so we pin `self` (the AdapterFormat Python + // object) onto every OrtValue. The patient list bumps self's + // Python refcount, so callers that keep any individual OrtValue + // keep the backing adapter alive. // - // We cannot attach py::keep_alive<0, 1> here: pybind11 keep_alive - // takes a weak reference to the returned object as the "patient", and - // Python `dict` does not support weak references, so the policy - // raises TypeError at call time. Lifetime safety for the documented - // user-facing pattern + // Why per-element pinning, not a property-level + // return_value_policy: pybind11 policies attach to the function's + // single return value. Setting reference_internal on this getter + // would try to pin `self` onto the returned `py::dict`, but + // (a) dict is not a pybind-registered type so keep_alive falls + // back to weakref and dicts are not weak-referenceable -> runtime + // TypeError, and (b) even if that worked, it would not propagate + // to the dict's elements, so a caller doing + // value = params["x"]; del params + // would still dangle. We therefore apply the policy per element, + // using the per-call form of py::cast: it requests + // reference_internal with `self` as parent, which routes through + // pybind11's add_patient on the OrtValue (a registered type) -- + // a strong refcount-based pin, not a weakref. // - // params = ort.AdapterFormat.read_adapter(path).get_parameters() + // The dict is built in a local first and only returned at the + // end, so a throw mid-build leaves no half-populated state. // - // is therefore provided by the Python wrapper - // (onnxruntime_inference_collection.AdapterFormat.get_parameters), - // which pins the owning C AdapterFormat on every returned OrtValue. - // This raw pybind11 class is an implementation detail; callers that - // bypass the wrapper and reach for `C.AdapterFormat(...).parameters` - // directly are responsible for keeping the AdapterFormat alive while - // they use the dict. - [](const PyAdapterFormatReaderWriter* reader_writer) -> py::dict { return reader_writer->parameters_; }, + // We deliberately do NOT cache the dict on `self`: caching would + // create a self -> dict -> OrtValue -> self reference cycle + // (via the patient list), and pybind11 instances are not + // traversable by the cyclic GC, so the cycle would leak. + [](py::object self) -> py::dict { + auto* rw = self.cast(); + if (!rw->loaded_adapter_.has_value()) { + // Write path: return whatever the user previously set. + return rw->parameters_; + } + py::dict params; + auto [begin, end] = rw->loaded_adapter_->GetParamIterators(); + for (; begin != end; ++begin) { + auto& [name, param] = *begin; + OrtValue& ort_value = param.GetMapped(); + // reference_internal == "borrowed reference owned by `self`"; + // pybind11 attaches `self` as a keep_alive patient on the + // returned OrtValue handle. + py::object ort_value_obj = py::cast( + &ort_value, py::return_value_policy::reference_internal, self); + params[py::str(name)] = std::move(ort_value_obj); + } + return params; + }, [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { reader_writer->parameters_ = parameters; }, @@ -108,10 +138,14 @@ void addAdapterFormatMethods(pybind11::module& m) { std::filesystem::path file_path(path); adapters::utils::AdapterFormatBuilder format_builder; - for (auto& [n, value] : reader_writer->parameters_) { - const std::string param_name = py::str(n); - const OrtValue* ort_value = value.cast(); - const Tensor& tensor = ort_value->Get(); + + // Visit every (name, OrtValue) pair from the appropriate source. + // We avoid building any intermediate Python dict here so the + // export path is exception-safe by construction: the builder + // accumulates in memory, and we only touch the filesystem + // after FinishWithSpan succeeds. + auto add_param = [&format_builder](const std::string& param_name, const OrtValue& ort_value) { + const Tensor& tensor = ort_value.Get(); const auto element_type = tensor.GetElementType(); // Reject string tensors: Tensor::DataRaw() for a string tensor points to an // array of std::string objects, and SizeInBytes() counts the sizeof(std::string) @@ -131,6 +165,20 @@ void addAdapterFormatMethods(pybind11::module& m) { format_builder.AddParameter( param_name, static_cast(element_type), tensor.Shape().GetDims(), data_span); + }; + + if (reader_writer->loaded_adapter_.has_value()) { + auto [begin, end] = reader_writer->loaded_adapter_->GetParamIterators(); + for (; begin != end; ++begin) { + auto& [name, param] = *begin; + add_param(name, param.GetMapped()); + } + } else { + for (auto& [n, value] : reader_writer->parameters_) { + const std::string param_name = py::str(n); + const OrtValue* ort_value = value.cast(); + add_param(param_name, *ort_value); + } } // Build the entire adapter image in memory before touching the @@ -159,20 +207,11 @@ void addAdapterFormatMethods(pybind11::module& m) { "read_adapter", [](const std::wstring& file_path) -> std::unique_ptr { lora::LoraAdapter lora_adapter; lora_adapter.Load(file_path); - - auto [begin, end] = lora_adapter.GetParamIterators(); - py::dict params; - for (; begin != end; ++begin) { - auto& [name, param] = *begin; - OrtValue& ort_value = param.GetMapped(); - params[py::str(name)] = py::cast(&ort_value); - } - - auto py_adapter = std::make_unique( - lora_adapter.FormatVersion(), lora_adapter.AdapterVersion(), - lora_adapter.ModelVersion(), std::move(lora_adapter), std::move(params)); - - return py_adapter; + const int format_version = lora_adapter.FormatVersion(); + const int adapter_version = lora_adapter.AdapterVersion(); + const int model_version = lora_adapter.ModelVersion(); + return std::make_unique( + format_version, adapter_version, model_version, std::move(lora_adapter)); }, R"pbdoc(The function returns an instance of the class that contains a dictionary of name -> numpy arrays)pbdoc"); diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index f53f674a012ea..4525b8a842f9a 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2104,13 +2104,15 @@ def test_adater_export_read(self): np.testing.assert_allclose(value.numpy(), expected_val.numpy()) def test_adapter_parameters_keep_alive(self): - # Regression test: AdapterFormat.get_parameters() returned OrtValue - # wrappers over non-owning C OrtValue* views into the parent adapter, - # with nothing keeping the parent alive. The natural pattern below - # dropped the parent and left the dict with dangling pointers, causing - # a use-after-free on the next access. The Python wrapper now pins the - # owning C AdapterFormat on every OrtValue it hands back, so callers - # that keep any of the values keep the backing adapter alive too. + # Regression test: AdapterFormat.read_adapter returned OrtValue views + # over storage owned by the C AdapterFormat with nothing keeping the + # parent alive. The natural pattern below dropped the parent and left + # the dict with dangling pointers, causing a use-after-free on the + # next access. read_adapter now pins the owning C AdapterFormat on + # every OrtValue it produces (pybind11 add_patient via + # keep_alive_impl), so the dict and any individual value keep the + # backing adapter alive on their own. Mirrors the strong-ref pattern + # used by the SparseTensor view bindings. adapter_version = 1 model_version = 1 file_path = pathlib.Path(os.path.realpath(__file__)).parent From b20e2c512cba327aec20e773869cb9be5c867085 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Mon, 8 Jun 2026 11:29:23 -0700 Subject: [PATCH 09/15] Validate tensor type and CPU location in export_adapter; mark holders noexcept - Add ORT_ENFORCE guards in export_adapter's add_param lambda to verify the OrtValue is a tensor and resides on CPU before calling DataRaw(). Previously, passing a GPU tensor or non-tensor OrtValue would crash with a device-pointer dereference on the host. - Mark BufferHolder and MemMapHolder constructors and move operations noexcept so the 'commit cannot throw' claim in Load()/MemoryMap() is compiler-verifiable via std::variant's noexcept guarantees. --- onnxruntime/core/session/lora_adapters.h | 8 ++++++-- onnxruntime/python/onnxruntime_pybind_lora.cc | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/onnxruntime/core/session/lora_adapters.h b/onnxruntime/core/session/lora_adapters.h index 7610779b2d69c..02c37a55726eb 100644 --- a/onnxruntime/core/session/lora_adapters.h +++ b/onnxruntime/core/session/lora_adapters.h @@ -160,13 +160,17 @@ class LoraAdapter { std::unordered_map BuildParamsValues(const adapters::Adapter* adapter) const; struct BufferHolder { - explicit BufferHolder(std::vector buffer) : buffer_(std::move(buffer)) {} + explicit BufferHolder(std::vector buffer) noexcept : buffer_(std::move(buffer)) {} + BufferHolder(BufferHolder&&) noexcept = default; + BufferHolder& operator=(BufferHolder&&) noexcept = default; std::vector buffer_; }; struct MemMapHolder { - MemMapHolder(Env::MappedMemoryPtr mapped_memory, size_t file_size) + MemMapHolder(Env::MappedMemoryPtr mapped_memory, size_t file_size) noexcept : mapped_memory_(std::move(mapped_memory)), file_size_(file_size) {} + MemMapHolder(MemMapHolder&&) noexcept = default; + MemMapHolder& operator=(MemMapHolder&&) noexcept = default; Env::MappedMemoryPtr mapped_memory_; size_t file_size_; }; diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index b4e876a3dafec..f322507aec54b 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -145,7 +145,11 @@ void addAdapterFormatMethods(pybind11::module& m) { // accumulates in memory, and we only touch the filesystem // after FinishWithSpan succeeds. auto add_param = [&format_builder](const std::string& param_name, const OrtValue& ort_value) { + ORT_ENFORCE(ort_value.IsTensor(), + "Lora adapter parameter '", param_name, "' must be a Tensor OrtValue."); const Tensor& tensor = ort_value.Get(); + ORT_ENFORCE(tensor.Location().device.Type() == OrtDevice::CPU, + "Lora adapter parameter '", param_name, "' must reside on CPU to be exported."); const auto element_type = tensor.GetElementType(); // Reject string tensors: Tensor::DataRaw() for a string tensor points to an // array of std::string objects, and SizeInBytes() counts the sizeof(std::string) From 79c175b60d5521f07c023561ca4a349e2c0381f6 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Mon, 8 Jun 2026 14:56:48 -0700 Subject: [PATCH 10/15] Fix behavioral regression: setter must clear loaded_adapter_ After read_adapter, calling the parameters setter followed by export_adapter would silently re-export the original read data because export_adapter prioritized loaded_adapter_ over the user-supplied parameters_ dict. Fix: clear loaded_adapter_ in the setter so that both the getter and export_adapter consistently use the user-supplied parameters after an explicit set. Add a regression test for the read -> modify -> export path. --- onnxruntime/python/onnxruntime_pybind_lora.cc | 5 +++ .../test/python/onnxruntime_test_python.py | 44 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index f322507aec54b..84c15856eef4c 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -129,6 +129,11 @@ void addAdapterFormatMethods(pybind11::module& m) { return params; }, [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { + // Clear loaded_adapter_ so subsequent getter and export_adapter + // operations use the user-supplied dict. Without this, a read → + // set_parameters → export sequence would silently ignore the new + // parameters and re-export the original read data. + reader_writer->loaded_adapter_.reset(); reader_writer->parameters_ = parameters; }, R"pbdoc("Enables user to read/write the dictionary of adapter parameters (name -> OrtValue)")pbdoc") diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 4525b8a842f9a..1979137719c45 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2103,6 +2103,50 @@ def test_adater_export_read(self): self.assertEqual(expected_val.shape(), value.shape()) np.testing.assert_allclose(value.numpy(), expected_val.numpy()) + def test_adapter_read_modify_export(self): + # Regression test: after read_adapter, calling set_parameters and then + # export_adapter must export the NEW parameters, not the originally read + # data. The setter must clear the internal loaded_adapter_ so that + # export_adapter uses the user-supplied dict. + adapter_version = 1 + model_version = 1 + file_path = pathlib.Path(os.path.realpath(__file__)).parent + original_path = str(file_path / "test_adapter_read_modify_original.onnx_adapter") + modified_path = str(file_path / "test_adapter_read_modify_new.onnx_adapter") + + float_data_type = 1 + original_data = np.array([1, 2, 3, 4]).astype(np.float32).reshape(2, 2) + modified_data = np.array([10, 20, 30, 40, 50, 60]).astype(np.float32).reshape(3, 2) + + ort_original = onnxrt.OrtValue.ortvalue_from_numpy_with_onnx_type(original_data, float_data_type) + ort_modified = onnxrt.OrtValue.ortvalue_from_numpy_with_onnx_type(modified_data, float_data_type) + + # Write original adapter + adapter_format = onnxrt.AdapterFormat() + adapter_format.set_adapter_version(adapter_version) + adapter_format.set_model_version(model_version) + adapter_format.set_parameters({"param": ort_original}) + adapter_format.export_adapter(original_path) + + try: + # Read, modify, and re-export + adapter_read = onnxrt.AdapterFormat.read_adapter(original_path) + adapter_read.set_parameters({"new_param": ort_modified}) + adapter_read.export_adapter(modified_path) + + # Verify the re-exported file has the NEW parameters + adapter_verify = onnxrt.AdapterFormat.read_adapter(modified_path) + params = adapter_verify.get_parameters() + self.assertIn("new_param", params) + self.assertNotIn("param", params) + self.assertEqual(params["new_param"].shape(), [3, 2]) + np.testing.assert_allclose(params["new_param"].numpy(), modified_data) + finally: + if os.path.exists(original_path): + os.remove(original_path) + if os.path.exists(modified_path): + os.remove(modified_path) + def test_adapter_parameters_keep_alive(self): # Regression test: AdapterFormat.read_adapter returned OrtValue views # over storage owned by the C AdapterFormat with nothing keeping the From 608f198f92a515928da881334e599bf67716a447 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Mon, 8 Jun 2026 15:02:39 -0700 Subject: [PATCH 11/15] Document two-source architecture rationale in PyAdapterFormatReaderWriter Move the design explanation into a class-level comment that explains: - WHY two sources: zero-copy views avoid duplicating adapter weights on memory-constrained devices. - WHY not a single cached dict: pybind11 instances lack tp_traverse, so self -> dict -> OrtValue (patient) -> self would be an un-collectable reference cycle. - HOW the setter bridges the two: clearing loaded_adapter_ on explicit set_parameters ensures consistent behavior. Trim verbose inline comments that repeated the same reasoning. --- onnxruntime/python/onnxruntime_pybind_lora.cc | 74 ++++++++----------- 1 file changed, 31 insertions(+), 43 deletions(-) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index 84c15856eef4c..69a4c03a5a7da 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -29,7 +29,27 @@ namespace py = pybind11; namespace { /// /// Class that supports writing and reading adapters -/// in innxruntime format +/// in onnxruntime format. +/// +/// Design: Two-source architecture (loaded_adapter_ vs parameters_) +/// +/// On the read path, OrtValues are zero-copy views into the LoraAdapter's +/// memory-mapped (or loaded) buffer. This avoids duplicating potentially +/// large adapter weights on memory-constrained devices. +/// +/// Unifying into a single cached py::dict is not possible because it would +/// create an un-collectable reference cycle: +/// self -> parameters_ dict -> OrtValue (pybind11 patient list) -> self +/// pybind11 instances do not implement tp_traverse, so Python's cyclic GC +/// cannot break this cycle and the object would leak. +/// +/// Instead: +/// - loaded_adapter_ holds backing memory; the getter builds a fresh dict +/// each call, pinning `self` on each OrtValue (no cycle since the dict +/// is not stored on self). +/// - parameters_ is used only on the write path (user-supplied values). +/// - The setter clears loaded_adapter_ so that after an explicit +/// set_parameters, both getter and export_adapter use the new dict. /// struct PyAdapterFormatReaderWriter { PyAdapterFormatReaderWriter() = default; @@ -44,16 +64,9 @@ struct PyAdapterFormatReaderWriter { int format_version_{adapters::kAdapterFormatVersion}; int adapter_version_{0}; int model_version_{0}; - // Populated only on the read path (read_adapter). When present, the - // `parameters` getter builds OrtValue views over its params_values_ map - // and pins this object as the keep_alive patient on each value. + // Read path: owns the backing memory for zero-copy OrtValue views. std::optional loaded_adapter_; - // Populated only on the write path (user-supplied via the `parameters` - // setter) and consumed by export_adapter. On the read path this stays - // empty; the getter builds a fresh dict from loaded_adapter_ each call, - // which avoids an AdapterFormat -> dict -> OrtValue -> AdapterFormat - // reference cycle that pybind11's patient list would not let the GC - // break. + // Write path: user-supplied dict of name -> OrtValue. py::dict parameters_; }; @@ -79,35 +92,12 @@ void addAdapterFormatMethods(pybind11::module& m) { R"pbdoc("Enables user to read/write model version this adapter was created for")pbdoc") .def_property( "parameters", - // Getter: build the dict on demand. On the read path each - // returned OrtValue is a non-owning view over storage owned by - // loaded_adapter_, so we pin `self` (the AdapterFormat Python - // object) onto every OrtValue. The patient list bumps self's - // Python refcount, so callers that keep any individual OrtValue - // keep the backing adapter alive. - // - // Why per-element pinning, not a property-level - // return_value_policy: pybind11 policies attach to the function's - // single return value. Setting reference_internal on this getter - // would try to pin `self` onto the returned `py::dict`, but - // (a) dict is not a pybind-registered type so keep_alive falls - // back to weakref and dicts are not weak-referenceable -> runtime - // TypeError, and (b) even if that worked, it would not propagate - // to the dict's elements, so a caller doing - // value = params["x"]; del params - // would still dangle. We therefore apply the policy per element, - // using the per-call form of py::cast: it requests - // reference_internal with `self` as parent, which routes through - // pybind11's add_patient on the OrtValue (a registered type) -- - // a strong refcount-based pin, not a weakref. - // - // The dict is built in a local first and only returned at the - // end, so a throw mid-build leaves no half-populated state. - // - // We deliberately do NOT cache the dict on `self`: caching would - // create a self -> dict -> OrtValue -> self reference cycle - // (via the patient list), and pybind11 instances are not - // traversable by the cyclic GC, so the cycle would leak. + // Getter: builds a fresh dict each call. On the read path, each + // OrtValue is a non-owning view into loaded_adapter_'s buffer. + // We pin `self` as a patient on each OrtValue (not on the dict, + // since dicts are not weak-referenceable and keep_alive would fail). + // Building fresh each call avoids storing the dict on self, which + // would create an un-collectable cycle (see class comment above). [](py::object self) -> py::dict { auto* rw = self.cast(); if (!rw->loaded_adapter_.has_value()) { @@ -119,9 +109,7 @@ void addAdapterFormatMethods(pybind11::module& m) { for (; begin != end; ++begin) { auto& [name, param] = *begin; OrtValue& ort_value = param.GetMapped(); - // reference_internal == "borrowed reference owned by `self`"; - // pybind11 attaches `self` as a keep_alive patient on the - // returned OrtValue handle. + // Pin `self` as patient on each OrtValue view. py::object ort_value_obj = py::cast( &ort_value, py::return_value_policy::reference_internal, self); params[py::str(name)] = std::move(ort_value_obj); @@ -230,4 +218,4 @@ void addAdapterFormatMethods(pybind11::module& m) { } } // namespace python -} // namespace onnxruntime \ No newline at end of file +} // namespace onnxruntime From d42f91a880b7e497fe60c4e6f6e976664a207a57 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Mon, 8 Jun 2026 15:05:34 -0700 Subject: [PATCH 12/15] Enforce read/write mode separation: reject set_parameters on read instances An AdapterFormat instance now operates in one of two exclusive modes: - READ mode (from read_adapter): parameters are read-only zero-copy views. Calling set_parameters raises RuntimeError. - WRITE mode (default constructor): user sets parameters and exports. This eliminates the ambiguous two-source conditional in export_adapter and prevents silent data loss where set_parameters would be ignored. Users who need to re-export with different parameters should create a new instance. Update regression test to verify the read-only enforcement. --- .../onnxruntime_inference_collection.py | 14 +++++-- onnxruntime/python/onnxruntime_pybind_lora.cc | 39 +++++++++---------- .../test/python/onnxruntime_test_python.py | 25 ++++-------- 3 files changed, 35 insertions(+), 43 deletions(-) diff --git a/onnxruntime/python/onnxruntime_inference_collection.py b/onnxruntime/python/onnxruntime_inference_collection.py index 5319d5d473d6b..212c9a37c3d69 100644 --- a/onnxruntime/python/onnxruntime_inference_collection.py +++ b/onnxruntime/python/onnxruntime_inference_collection.py @@ -104,13 +104,19 @@ def get_model_version(self) -> int: return self._adapter.model_version def set_parameters(self, params: dict[str, OrtValue]) -> None: + """Set adapter parameters. Only valid on instances created via the default constructor. + + Raises RuntimeError if called on an instance returned by read_adapter(). + To re-export with different parameters, create a new AdapterFormat(). + """ self._adapter.parameters = {k: v._ortvalue for k, v in params.items()} def get_parameters(self) -> dict[str, OrtValue]: - # Each call to the C getter rebuilds the dict and pins the owning - # C AdapterFormat onto every returned OrtValue (pybind11 add_patient - # via keep_alive_impl), so callers can drop this AdapterFormat and - # the dict (or any individual value) keeps the backing adapter alive. + """Get adapter parameters as a dict of name -> OrtValue. + + On read instances, the returned OrtValues are zero-copy views that keep + the backing adapter memory alive as long as any value is referenced. + """ return {k: OrtValue(v) for k, v in self._adapter.parameters.items()} diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index 69a4c03a5a7da..f47f7834da919 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -31,25 +31,23 @@ namespace { /// Class that supports writing and reading adapters /// in onnxruntime format. /// -/// Design: Two-source architecture (loaded_adapter_ vs parameters_) +/// Design: An instance operates in one of two mutually exclusive modes: /// -/// On the read path, OrtValues are zero-copy views into the LoraAdapter's -/// memory-mapped (or loaded) buffer. This avoids duplicating potentially -/// large adapter weights on memory-constrained devices. +/// 1. READ mode — created by read_adapter(). OrtValues are zero-copy views +/// into the LoraAdapter's memory-mapped buffer. Parameters are read-only; +/// calling the setter throws. export_adapter() re-serializes from the +/// loaded adapter. /// -/// Unifying into a single cached py::dict is not possible because it would -/// create an un-collectable reference cycle: -/// self -> parameters_ dict -> OrtValue (pybind11 patient list) -> self -/// pybind11 instances do not implement tp_traverse, so Python's cyclic GC -/// cannot break this cycle and the object would leak. +/// 2. WRITE mode — created by the default constructor. The user sets +/// parameters (owning OrtValues) and calls export_adapter(). /// -/// Instead: -/// - loaded_adapter_ holds backing memory; the getter builds a fresh dict -/// each call, pinning `self` on each OrtValue (no cycle since the dict -/// is not stored on self). -/// - parameters_ is used only on the write path (user-supplied values). -/// - The setter clears loaded_adapter_ so that after an explicit -/// set_parameters, both getter and export_adapter use the new dict. +/// This separation avoids: +/// - Copying adapter weights (wasteful on memory-constrained devices). +/// - An un-collectable reference cycle (self -> cached dict -> OrtValue +/// patient list -> self) that pybind11's non-traversable instances would +/// leak. +/// - Silent behavioral regression where set_parameters on a read instance +/// would be ignored by export_adapter. /// struct PyAdapterFormatReaderWriter { PyAdapterFormatReaderWriter() = default; @@ -117,11 +115,10 @@ void addAdapterFormatMethods(pybind11::module& m) { return params; }, [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { - // Clear loaded_adapter_ so subsequent getter and export_adapter - // operations use the user-supplied dict. Without this, a read → - // set_parameters → export sequence would silently ignore the new - // parameters and re-export the original read data. - reader_writer->loaded_adapter_.reset(); + if (reader_writer->loaded_adapter_.has_value()) { + ORT_THROW("Cannot set parameters on an AdapterFormat instance created by read_adapter(). " + "Create a new AdapterFormat() for export instead."); + } reader_writer->parameters_ = parameters; }, R"pbdoc("Enables user to read/write the dictionary of adapter parameters (name -> OrtValue)")pbdoc") diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 1979137719c45..5fd833f4c60a8 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2104,15 +2104,13 @@ def test_adater_export_read(self): np.testing.assert_allclose(value.numpy(), expected_val.numpy()) def test_adapter_read_modify_export(self): - # Regression test: after read_adapter, calling set_parameters and then - # export_adapter must export the NEW parameters, not the originally read - # data. The setter must clear the internal loaded_adapter_ so that - # export_adapter uses the user-supplied dict. + # Regression test: an instance created by read_adapter is read-only. + # Attempting to set_parameters on it must raise, preventing silent + # data loss where the setter would be ignored by export_adapter. adapter_version = 1 model_version = 1 file_path = pathlib.Path(os.path.realpath(__file__)).parent original_path = str(file_path / "test_adapter_read_modify_original.onnx_adapter") - modified_path = str(file_path / "test_adapter_read_modify_new.onnx_adapter") float_data_type = 1 original_data = np.array([1, 2, 3, 4]).astype(np.float32).reshape(2, 2) @@ -2129,23 +2127,14 @@ def test_adapter_read_modify_export(self): adapter_format.export_adapter(original_path) try: - # Read, modify, and re-export + # Read instance is read-only; set_parameters must raise. adapter_read = onnxrt.AdapterFormat.read_adapter(original_path) - adapter_read.set_parameters({"new_param": ort_modified}) - adapter_read.export_adapter(modified_path) - - # Verify the re-exported file has the NEW parameters - adapter_verify = onnxrt.AdapterFormat.read_adapter(modified_path) - params = adapter_verify.get_parameters() - self.assertIn("new_param", params) - self.assertNotIn("param", params) - self.assertEqual(params["new_param"].shape(), [3, 2]) - np.testing.assert_allclose(params["new_param"].numpy(), modified_data) + with self.assertRaises(Exception) as ctx: + adapter_read.set_parameters({"new_param": ort_modified}) + self.assertIn("read_adapter", str(ctx.exception)) finally: if os.path.exists(original_path): os.remove(original_path) - if os.path.exists(modified_path): - os.remove(modified_path) def test_adapter_parameters_keep_alive(self): # Regression test: AdapterFormat.read_adapter returned OrtValue views From 5f17f6c240be20cc89d8baac23a4513bbdaccc83 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Mon, 8 Jun 2026 15:12:35 -0700 Subject: [PATCH 13/15] Replace two-source architecture with capsule-based single source Eliminate loaded_adapter_ / parameters_ duality entirely. The parameters_ dict is now always the single authoritative source. On the read path, a heap-allocated LoraAdapter is transferred into a py::capsule. Each OrtValue view in the dict has the capsule pinned as its pybind11 patient. The reference graph is: PyAdapterFormatReaderWriter -> parameters_ dict -> OrtValues -> capsule -> LoraAdapter No edge points back to PyAdapterFormatReaderWriter, so there is no reference cycle (pybind11 instances lack tp_traverse). The capsule ref-counts keep the backing memory alive as long as any OrtValue is referenced in Python. Benefits: - Zero-copy: no data duplication on memory-constrained devices. - No reference cycles: capsule is an opaque Python object the GC handles. - Single source: getter, setter, and export_adapter all operate on parameters_. read -> modify -> export just works. - Simpler code: removed conditional branches in getter and export_adapter. --- .../onnxruntime_inference_collection.py | 10 +- onnxruntime/python/onnxruntime_pybind_lora.cc | 126 ++++++++---------- .../test/python/onnxruntime_test_python.py | 23 +++- 3 files changed, 74 insertions(+), 85 deletions(-) diff --git a/onnxruntime/python/onnxruntime_inference_collection.py b/onnxruntime/python/onnxruntime_inference_collection.py index 212c9a37c3d69..70f699768e92c 100644 --- a/onnxruntime/python/onnxruntime_inference_collection.py +++ b/onnxruntime/python/onnxruntime_inference_collection.py @@ -104,18 +104,14 @@ def get_model_version(self) -> int: return self._adapter.model_version def set_parameters(self, params: dict[str, OrtValue]) -> None: - """Set adapter parameters. Only valid on instances created via the default constructor. - - Raises RuntimeError if called on an instance returned by read_adapter(). - To re-export with different parameters, create a new AdapterFormat(). - """ + """Set adapter parameters for export.""" self._adapter.parameters = {k: v._ortvalue for k, v in params.items()} def get_parameters(self) -> dict[str, OrtValue]: """Get adapter parameters as a dict of name -> OrtValue. - On read instances, the returned OrtValues are zero-copy views that keep - the backing adapter memory alive as long as any value is referenced. + On read instances, the returned OrtValues are zero-copy views; the + backing memory stays alive as long as any returned OrtValue is referenced. """ return {k: OrtValue(v) for k, v in self._adapter.parameters.items()} diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index f47f7834da919..75b5a4ad8820b 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -31,40 +31,37 @@ namespace { /// Class that supports writing and reading adapters /// in onnxruntime format. /// -/// Design: An instance operates in one of two mutually exclusive modes: +/// Design: Single-source architecture using py::capsule for memory ownership. /// -/// 1. READ mode — created by read_adapter(). OrtValues are zero-copy views -/// into the LoraAdapter's memory-mapped buffer. Parameters are read-only; -/// calling the setter throws. export_adapter() re-serializes from the -/// loaded adapter. +/// `parameters_` is always the single authoritative dict of name -> OrtValue. +/// On the read path, OrtValues are zero-copy views into a LoraAdapter's buffer. +/// The LoraAdapter is owned by a py::capsule that is pinned as a patient on each +/// OrtValue via pybind11's add_patient. The object graph is: /// -/// 2. WRITE mode — created by the default constructor. The user sets -/// parameters (owning OrtValues) and calls export_adapter(). +/// PyAdapterFormatReaderWriter -> parameters_ dict -> OrtValues -> capsule -> LoraAdapter /// -/// This separation avoids: -/// - Copying adapter weights (wasteful on memory-constrained devices). -/// - An un-collectable reference cycle (self -> cached dict -> OrtValue -/// patient list -> self) that pybind11's non-traversable instances would -/// leak. -/// - Silent behavioral regression where set_parameters on a read instance -/// would be ignored by export_adapter. +/// No edge points back to PyAdapterFormatReaderWriter, so there is no reference +/// cycle (pybind11 instances are not GC-traversable, so a cycle would leak). +/// The capsule keeps the backing memory alive as long as any OrtValue referencing +/// it is alive. +/// +/// The setter replaces parameters_ freely. Old OrtValues (and their capsule +/// patient) remain alive as long as Python references to them exist. /// struct PyAdapterFormatReaderWriter { PyAdapterFormatReaderWriter() = default; PyAdapterFormatReaderWriter(int format_version, int adapter_version, int model_version, - lora::LoraAdapter&& loaded_adapter) + py::dict parameters) : format_version_(format_version), adapter_version_(adapter_version), model_version_(model_version), - loaded_adapter_(std::move(loaded_adapter)) {} + parameters_(std::move(parameters)) {} int format_version_{adapters::kAdapterFormatVersion}; int adapter_version_{0}; int model_version_{0}; - // Read path: owns the backing memory for zero-copy OrtValue views. - std::optional loaded_adapter_; - // Write path: user-supplied dict of name -> OrtValue. + // Single source of truth: name -> OrtValue (views or owning). py::dict parameters_; }; @@ -90,36 +87,11 @@ void addAdapterFormatMethods(pybind11::module& m) { R"pbdoc("Enables user to read/write model version this adapter was created for")pbdoc") .def_property( "parameters", - // Getter: builds a fresh dict each call. On the read path, each - // OrtValue is a non-owning view into loaded_adapter_'s buffer. - // We pin `self` as a patient on each OrtValue (not on the dict, - // since dicts are not weak-referenceable and keep_alive would fail). - // Building fresh each call avoids storing the dict on self, which - // would create an un-collectable cycle (see class comment above). - [](py::object self) -> py::dict { - auto* rw = self.cast(); - if (!rw->loaded_adapter_.has_value()) { - // Write path: return whatever the user previously set. - return rw->parameters_; - } - py::dict params; - auto [begin, end] = rw->loaded_adapter_->GetParamIterators(); - for (; begin != end; ++begin) { - auto& [name, param] = *begin; - OrtValue& ort_value = param.GetMapped(); - // Pin `self` as patient on each OrtValue view. - py::object ort_value_obj = py::cast( - &ort_value, py::return_value_policy::reference_internal, self); - params[py::str(name)] = std::move(ort_value_obj); - } - return params; + [](const PyAdapterFormatReaderWriter* reader_writer) -> py::dict { + return reader_writer->parameters_; }, - [](PyAdapterFormatReaderWriter* reader_writer, py::dict& parameters) -> void { - if (reader_writer->loaded_adapter_.has_value()) { - ORT_THROW("Cannot set parameters on an AdapterFormat instance created by read_adapter(). " - "Create a new AdapterFormat() for export instead."); - } - reader_writer->parameters_ = parameters; + [](PyAdapterFormatReaderWriter* reader_writer, py::dict parameters) -> void { + reader_writer->parameters_ = std::move(parameters); }, R"pbdoc("Enables user to read/write the dictionary of adapter parameters (name -> OrtValue)")pbdoc") .def( @@ -129,11 +101,6 @@ void addAdapterFormatMethods(pybind11::module& m) { adapters::utils::AdapterFormatBuilder format_builder; - // Visit every (name, OrtValue) pair from the appropriate source. - // We avoid building any intermediate Python dict here so the - // export path is exception-safe by construction: the builder - // accumulates in memory, and we only touch the filesystem - // after FinishWithSpan succeeds. auto add_param = [&format_builder](const std::string& param_name, const OrtValue& ort_value) { ORT_ENFORCE(ort_value.IsTensor(), "Lora adapter parameter '", param_name, "' must be a Tensor OrtValue."); @@ -161,18 +128,11 @@ void addAdapterFormatMethods(pybind11::module& m) { tensor.Shape().GetDims(), data_span); }; - if (reader_writer->loaded_adapter_.has_value()) { - auto [begin, end] = reader_writer->loaded_adapter_->GetParamIterators(); - for (; begin != end; ++begin) { - auto& [name, param] = *begin; - add_param(name, param.GetMapped()); - } - } else { - for (auto& [n, value] : reader_writer->parameters_) { - const std::string param_name = py::str(n); - const OrtValue* ort_value = value.cast(); - add_param(param_name, *ort_value); - } + // Single source: always iterate parameters_ dict. + for (auto& [n, value] : reader_writer->parameters_) { + const std::string param_name = py::str(n); + const OrtValue* ort_value = value.cast(); + add_param(param_name, *ort_value); } // Build the entire adapter image in memory before touching the @@ -199,13 +159,37 @@ void addAdapterFormatMethods(pybind11::module& m) { .def_static( "read_adapter", [](const std::wstring& file_path) -> std::unique_ptr { - lora::LoraAdapter lora_adapter; - lora_adapter.Load(file_path); - const int format_version = lora_adapter.FormatVersion(); - const int adapter_version = lora_adapter.AdapterVersion(); - const int model_version = lora_adapter.ModelVersion(); + // Load into a heap-allocated LoraAdapter, then wrap it in a capsule. + // The capsule is pinned as a patient on each OrtValue view, so the + // backing memory stays alive as long as any Python reference to an + // OrtValue (or the dict containing them) exists. + auto adapter_ptr = std::make_unique(); + adapter_ptr->Load(file_path); + const int format_version = adapter_ptr->FormatVersion(); + const int adapter_version = adapter_ptr->AdapterVersion(); + const int model_version = adapter_ptr->ModelVersion(); + + // Transfer ownership of the LoraAdapter to a capsule. + lora::LoraAdapter* raw_adapter = adapter_ptr.release(); + py::capsule adapter_capsule(raw_adapter, [](void* p) { + delete static_cast(p); + }); + + // Build the parameters dict with OrtValue views pinned to the capsule. + py::dict params; + auto [begin, end] = raw_adapter->GetParamIterators(); + for (; begin != end; ++begin) { + auto& [name, param] = *begin; + OrtValue& ort_value = param.GetMapped(); + // Cast with the capsule as parent: pybind11 attaches the capsule + // as a patient on the OrtValue handle (strong refcount pin). + py::object ort_value_obj = py::cast( + &ort_value, py::return_value_policy::reference_internal, adapter_capsule); + params[py::str(name)] = std::move(ort_value_obj); + } + return std::make_unique( - format_version, adapter_version, model_version, std::move(lora_adapter)); + format_version, adapter_version, model_version, std::move(params)); }, R"pbdoc(The function returns an instance of the class that contains a dictionary of name -> numpy arrays)pbdoc"); diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 5fd833f4c60a8..7f2d61bc7934c 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2104,13 +2104,13 @@ def test_adater_export_read(self): np.testing.assert_allclose(value.numpy(), expected_val.numpy()) def test_adapter_read_modify_export(self): - # Regression test: an instance created by read_adapter is read-only. - # Attempting to set_parameters on it must raise, preventing silent - # data loss where the setter would be ignored by export_adapter. + # Verify that an instance created by read_adapter can have its + # parameters replaced and re-exported (single-source architecture). adapter_version = 1 model_version = 1 file_path = pathlib.Path(os.path.realpath(__file__)).parent original_path = str(file_path / "test_adapter_read_modify_original.onnx_adapter") + modified_path = str(file_path / "test_adapter_read_modify_new.onnx_adapter") float_data_type = 1 original_data = np.array([1, 2, 3, 4]).astype(np.float32).reshape(2, 2) @@ -2127,14 +2127,23 @@ def test_adapter_read_modify_export(self): adapter_format.export_adapter(original_path) try: - # Read instance is read-only; set_parameters must raise. + # Read, replace parameters, and re-export. adapter_read = onnxrt.AdapterFormat.read_adapter(original_path) - with self.assertRaises(Exception) as ctx: - adapter_read.set_parameters({"new_param": ort_modified}) - self.assertIn("read_adapter", str(ctx.exception)) + adapter_read.set_parameters({"new_param": ort_modified}) + adapter_read.export_adapter(modified_path) + + # Verify the re-exported file has the NEW parameters. + adapter_verify = onnxrt.AdapterFormat.read_adapter(modified_path) + params = adapter_verify.get_parameters() + self.assertIn("new_param", params) + self.assertNotIn("param", params) + self.assertEqual(params["new_param"].shape(), [3, 2]) + np.testing.assert_allclose(params["new_param"].numpy(), modified_data) finally: if os.path.exists(original_path): os.remove(original_path) + if os.path.exists(modified_path): + os.remove(modified_path) def test_adapter_parameters_keep_alive(self): # Regression test: AdapterFormat.read_adapter returned OrtValue views From 57de8ec3abac31d8c11ad6e234ad8e2895741f46 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Mon, 8 Jun 2026 16:52:00 -0700 Subject: [PATCH 14/15] Fix leak on capsule allocation failure (review feedback from tianleiwu) Construct py::capsule while unique_ptr still owns the LoraAdapter. If capsule allocation throws (e.g. bad_alloc), unique_ptr's destructor still cleans up. Only release() after capsule is successfully constructed. --- onnxruntime/python/onnxruntime_pybind_lora.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index 75b5a4ad8820b..5377bb883f8f9 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -170,10 +170,12 @@ void addAdapterFormatMethods(pybind11::module& m) { const int model_version = adapter_ptr->ModelVersion(); // Transfer ownership of the LoraAdapter to a capsule. - lora::LoraAdapter* raw_adapter = adapter_ptr.release(); - py::capsule adapter_capsule(raw_adapter, [](void* p) { + // Construct capsule while unique_ptr still owns the pointer, so + // if capsule allocation throws, unique_ptr cleans up. + py::capsule adapter_capsule(adapter_ptr.get(), [](void* p) { delete static_cast(p); }); + lora::LoraAdapter* raw_adapter = adapter_ptr.release(); // Build the parameters dict with OrtValue views pinned to the capsule. py::dict params; From fa401e32ec6276c60061d05c477c9f3cdae09563 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Tue, 9 Jun 2026 10:56:41 -0700 Subject: [PATCH 15/15] Address remaining PR review comments - read_adapter docstring updated to say 'name -> OrtValue' instead of 'name -> numpy arrays' (the binding has long returned OrtValue handles, and the Python wrapper exposes OrtValue objects). Aligns the inline doc with the actual API. - test_adapter_parameters_keep_alive: guard the finally-block os.remove with os.path.exists so an earlier failure (e.g. export_adapter raising) is not masked by a follow-on FileNotFoundError. Matches the pattern used in test_adapter_export_rejects_string_tensors. --- onnxruntime/python/onnxruntime_pybind_lora.cc | 2 +- onnxruntime/test/python/onnxruntime_test_python.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/onnxruntime/python/onnxruntime_pybind_lora.cc b/onnxruntime/python/onnxruntime_pybind_lora.cc index 5377bb883f8f9..7c6608a4f9052 100644 --- a/onnxruntime/python/onnxruntime_pybind_lora.cc +++ b/onnxruntime/python/onnxruntime_pybind_lora.cc @@ -193,7 +193,7 @@ void addAdapterFormatMethods(pybind11::module& m) { return std::make_unique( format_version, adapter_version, model_version, std::move(params)); }, - R"pbdoc(The function returns an instance of the class that contains a dictionary of name -> numpy arrays)pbdoc"); + R"pbdoc(The function returns an instance of the class that contains a dictionary of name -> OrtValue)pbdoc"); py::class_ lora_adapter_binding(m, "LoraAdapter"); lora_adapter_binding.def(py::init()) diff --git a/onnxruntime/test/python/onnxruntime_test_python.py b/onnxruntime/test/python/onnxruntime_test_python.py index 7f2d61bc7934c..bebd290bbe810 100644 --- a/onnxruntime/test/python/onnxruntime_test_python.py +++ b/onnxruntime/test/python/onnxruntime_test_python.py @@ -2190,7 +2190,8 @@ def test_adapter_parameters_keep_alive(self): gc.collect() np.testing.assert_allclose(single_value.numpy(), param_1) finally: - os.remove(file_path) + if os.path.exists(file_path): + os.remove(file_path) def test_adapter_export_rejects_string_tensors(self): # Regression test: export_adapter previously serialized Tensor::DataRaw()