Consolidate bindings for Enterprise ABI compatibility #2821

phoebusm · 2025-12-19T19:31:09Z

Reference Issues/PRs

Part of https://man312219.monday.com/boards/7852509418/pulses/18378732140 and
https://man312219.monday.com/boards/7852509418/pulses/8371158618

What does this implement or fix?

Consolidate some binding functions so they can be reused in different repos
Fix sts fixtures missing configuration if the native version store is reused
( Now add_s3_library_to_env set both native config and class variable for config aws_auth and aws_profile. They should only be set in native config. It can't be changed until 7.0.0)

Any other comments?

Checklist

Checklist for code changes...

Have you updated the relevant docstrings, documentation and copyright notice?
Is this contribution tested against all ArcticDB's features?
Do all exceptions introduced raise appropriate error messages?
Are API changes highlighted in the PR description?
Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

IvoDD · 2025-12-29T13:27:14Z

cpp/arcticdb/storage/python_bindings_common.hpp

+s3::S3Settings s3_settings(const pybind11::tuple& t);
+pybind11::tuple to_tuple(const s3::GCPXMLSettings& settings);
+pybind11::tuple to_tuple(const s3::S3Settings& settings);
+void register_common_bindings(pybind11::module& m, bool local_bindings);


Nit: Maybe rename to register_common_storage_bindings?

vasil-pashov · 2026-01-06T10:27:36Z

cpp/arcticdb/entity/python_bindings_common.cpp

+                    [](py::tuple t) {
+                        util::check(t.size() >= 7, "Invalid AtomKey pickle object!");
+
+                        [[maybe_unused]] const int serialization_version = t[0].cast<int>();


Is this needed? Why don't we remove it?

vasil-pashov · 2026-01-06T10:30:43Z

cpp/arcticdb/entity/python_bindings_common.cpp

+                        );
+                    },
+                    [](py::tuple t) {
+                        util::check(t.size() >= 7, "Invalid AtomKey pickle object!");


Why not check for t.size() == 7

I think it's for some sort of future proof, if new setting is added to the class.

It's for the staged data tokens API, which makes the staged data tokens (which wrap atom keys) pickleable. This helps to use the staged data API across processes which may have different ArcticDB versions.

vasil-pashov · 2026-01-06T10:32:16Z

cpp/arcticdb/entity/python_bindings_common.hpp

+
+#pragma once
+
+#include <pybind11/pybind11.h>


You don't need this header in the .hpp file. You can forward declare pybind11::module and place the header only in the .cpp

That'll work as well. Is it for the sake of compilation speed?

vasil-pashov · 2026-01-06T11:29:03Z

cpp/arcticdb/python/python_bindings_common.hpp

@@ -0,0 +1,13 @@
+#pragma once
+#include <pybind11/pybind11.h>


same as https://github.com/man-group/ArcticDB/pull/2821/changes#r2664451476

vasil-pashov · 2026-01-06T11:56:18Z

cpp/arcticdb/entity/python_bindings_common.cpp

+namespace arcticdb::entity::apy {
+
+void register_common_entity_bindings(py::module& m, BindingScope scope) {
+    bool local_bindings = (scope == BindingScope::LOCAL);


register_common_entity_bindings is only called with BindingScope::GLOBAL why does it have an option to pass LOCAL?

I've probably missed some part of the discussion. So I wanted to double check the intent.

We will distinguish ArcticDB and ArcticDB Enterprise AtomKey and VersionItem? ArcticDB Enterprise users won't be able to pass in AtomKey return from ArcticDB.

This will also mean the the library tool currently won't be able to operate with AtomKeys from Enterprise.

register_common_entity_bindings is only called with BindingScope::GLOBAL why does it have an option to pass LOCAL?

It used by downstream library, namely Enterprise.

We will distinguish ArcticDB and ArcticDB Enterprise AtomKey and VersionItem? ArcticDB Enterprise users won't be able to pass in AtomKey return from ArcticDB.

It's never supposed to work out of the box as they are defined in different binaries.
The reason why they are needed to be bound in enterprise, as they are the return type of some functions. If core and enterprise are ABI-compatible, not binding those class will give us pybind exception.
But it will be very useful to provide some helper functions to convert those two types between two binaries

vasil-pashov · 2026-01-06T12:02:27Z

cpp/arcticdb/storage/python_bindings_common.hpp

+#include <pybind11/pybind11.h>
+#include <arcticdb/util/error_code.hpp>
+#include <arcticdb/storage/s3/s3_settings.hpp>
+#include <arcticdb/storage/common.hpp>
+#include <arcticdb/python/python_bindings_common.hpp>


We can trim some of the includes using fwd declarations.

poodlewars · 2025-12-29T09:01:57Z

cpp/arcticdb/entity/python_bindings_common.hpp

+
+namespace arcticdb::entity::apy {
+
+void register_common_entity_bindings(pybind11::module& m, bool local_bindings);


What does "common" mean here?

poodlewars · 2025-12-29T09:06:53Z

cpp/arcticdb/entity/python_bindings_common.hpp

+
+namespace arcticdb::entity::apy {
+
+void register_common_entity_bindings(pybind11::module& m, bool local_bindings);


We should avoid bool flags on APIs, it's hard to guess what the false in entity::apy::register_common_entity_bindings(version, false); means. Instead there should be separate functions for the true and false cases, which might push down to a private function with the bool in its signature.

poodlewars · 2025-12-29T09:18:12Z

python/arcticdb/storage_fixtures/s3.py

        logger.error(f"Error creating access key: {e}")
        raise e

+    out.native_config = NativeVariantStorage(


I don't understand this change, although I've seen your comment,

Fix sts fixtures missing configuration if the native version store is reused ( Now add_s3_library_to_env set both native config and class variable for config aws_auth and aws_profile. They should only be set in native config. It can't be changed until 7.0.0)

What does if the native version store is reused mean? A unit test to go with this change would make this easier to review, and help us avoid breaking it again.

poodlewars · 2025-12-29T09:19:42Z

python/arcticdb/storage_fixtures/s3.py

        raise e

+    out.native_config = NativeVariantStorage(
+        NativeS3Settings(AWSAuthMethod.STS_PROFILE_CREDENTIALS_PROVIDER, profile_name, False)


Can we use kwargs for the last argument whenever we create this object? This also has the True / False readability problem.

poodlewars · 2026-01-09T20:46:22Z

python/tests/compat/arcticdb/test_core_api.py

These unit tests don't cover the _core_api.py functions?

poodlewars · 2026-01-09T20:47:09Z

python/arcticdb/_core_api.py

+Copyright 2023 Man Group Operations Ltd.
+NO WARRANTY, EXPRESSED OR IMPLIED.
+
+This module implements a backwards compatible version of msgpack functions.


poodlewars · 2026-01-09T20:47:30Z

python/arcticdb/_core_api.py

+This module implements a backwards compatible version of msgpack functions.
+"""
+
+# Treat everything here as public APIs!!!! Any change made here may break downstream repos!!!!


Worth mentioning that we should apply semantic versioning here

poodlewars · 2026-01-09T20:52:39Z

cpp/arcticdb/python/python_bindings_common.cpp

Why do we need to make the metrics config pickleable? This relates to this comment on the enterprise PR https://github.com/man-group/arcticdb-enterprise/pull/281#discussion_r2650734598 . All we need is to add accessors to the fields in hte metrics config? It's confusing to use the pickling APIs for this purpose.

phoebusm added patch Small change, should increase patch version no-release-notes This PR shouldn't be added to release notes. labels Dec 19, 2025

phoebusm changed the title ~~For Enterprise ABI compatibility v2~~ For Enterprise ABI compatibility Dec 19, 2025

phoebusm changed the title ~~For Enterprise ABI compatibility~~ Consolidate bindings for Enterprise ABI compatibility Dec 24, 2025

phoebusm marked this pull request as ready for review December 24, 2025 17:32

phoebusm requested review from IvoDD, alexowens90 and poodlewars as code owners December 24, 2025 17:32

phoebusm removed the no-release-notes This PR shouldn't be added to release notes. label Dec 24, 2025

Extract bindings functions so that they could be reused elsewhere

be18e9b

phoebusm force-pushed the enterprise_abi_v2 branch from aaeb3f9 to be18e9b Compare December 24, 2025 17:42

IvoDD reviewed Dec 29, 2025

View reviewed changes

vasil-pashov reviewed Jan 6, 2026

View reviewed changes

Address PR comments

dc8a9c7

phoebusm force-pushed the enterprise_abi_v2 branch from 6cecabc to dc8a9c7 Compare January 9, 2026 14:39

poodlewars reviewed Jan 9, 2026

View reviewed changes

python/tests/compat/arcticdb/test_core_api.py

Copy link

Collaborator

poodlewars Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These unit tests don't cover the _core_api.py functions?

poodlewars reviewed Jan 9, 2026

View reviewed changes


		namespace arcticdb::entity::apy {

		void register_common_entity_bindings(pybind11::module& m, bool local_bindings);


		#pragma once

		#include <pybind11/pybind11.h>

Consolidate bindings for Enterprise ABI compatibility #2821

Are you sure you want to change the base?

Consolidate bindings for Enterprise ABI compatibility #2821

Uh oh!

Conversation

phoebusm commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoebusm Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoebusm Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poodlewars Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

phoebusm commented Dec 19, 2025 •

edited

Loading

phoebusm Jan 8, 2026 •

edited

Loading

phoebusm Jan 8, 2026 •

edited

Loading

poodlewars Jan 9, 2026 •

edited

Loading