-
Notifications
You must be signed in to change notification settings - Fork 161
Consolidate bindings for Enterprise ABI compatibility #2821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
be18e9b
dc8a9c7
9f92afd
6251ddd
fd875bb
3dccc3c
3cc34ee
12fb59b
2e44216
9f6f24f
8ac58da
972319d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,8 @@ namespace py = pybind11; | |
|
|
||
| namespace arcticdb::entity::apy { | ||
|
|
||
| void register_common_entity_bindings(py::module& m, bool local_bindings) { | ||
| void register_common_entity_bindings(py::module& m, BindingScope scope) { | ||
| bool local_bindings = (scope == BindingScope::LOCAL); | ||
| py::class_<AtomKey, std::shared_ptr<AtomKey>>(m, "AtomKey", py::module_local(local_bindings)) | ||
| .def(py::init()) | ||
| .def(py::init<StreamId, VersionId, timestamp, ContentHash, IndexValue, IndexValue, KeyType>()) | ||
|
|
@@ -52,7 +53,6 @@ void register_common_entity_bindings(py::module& m, bool local_bindings) { | |
| [](py::tuple t) { | ||
| util::check(t.size() >= 7, "Invalid AtomKey pickle object!"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not check for
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's for some sort of future proof, if new setting is added to the class.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| [[maybe_unused]] const int serialization_version = t[0].cast<int>(); | ||
| AtomKey key( | ||
| t[1].cast<StreamId>(), | ||
| t[2].cast<VersionId>(), | ||
|
|
||
poodlewars marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,18 @@ | ||
| #pragma once | ||
| #include <pybind11/pybind11.h> | ||
| #include <arcticdb/entity/metrics.hpp> | ||
|
|
||
| namespace arcticdb::apy { | ||
| namespace pybind11 { | ||
| class module_; | ||
| using module = module_; | ||
| class tuple; | ||
| } // namespace pybind11 | ||
|
|
||
| enum class MetricsConfigPickleOrder : uint32_t { | ||
| HOST = 0, | ||
| PORT = 1, | ||
| JOB_NAME = 2, | ||
| INSTANCE = 3, | ||
| PROMETHEUS_ENV = 4, | ||
| MODEL = 5 | ||
| }; | ||
| namespace arcticdb { | ||
| enum class BindingScope : uint32_t { LOCAL = 0, GLOBAL = 1 }; | ||
|
|
||
| pybind11::tuple to_tuple(const MetricsConfig& config); | ||
| MetricsConfig metrics_config_from_tuple(const pybind11::tuple& t); | ||
|
|
||
| } // namespace arcticdb::apy | ||
| } // namespace arcticdb | ||
|
|
||
| void register_metrics(pybind11::module&& m, bool local_bindings); | ||
| void register_metrics(pybind11::module&& m, arcticdb::BindingScope scope); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| """ | ||
| Copyright 2023 Man Group Operations Ltd. | ||
| NO WARRANTY, EXPRESSED OR IMPLIED. | ||
|
|
||
| This module implements a backwards compatible version of msgpack functions. | ||
poodlewars marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| """ | ||
|
|
||
| # Treat everything here as public APIs!!!! Any change made here may break downstream repos!!!! | ||
poodlewars marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| from arcticdb_ext.storage import NativeVariantStorage | ||
| from arcticdb_ext.metrics.prometheus import MetricsConfig | ||
| from arcticdb.version_store._store import _env_config_from_lib_config as env_config_from_lib_config | ||
|
|
||
|
|
||
| def convert_native_variant_storage_to_py_tuple(native_cfg: NativeVariantStorage): | ||
| return native_cfg.__getstate__() | ||
|
|
||
|
|
||
| def convert_metrics_config_to_py_tuple(metrics_cfg: MetricsConfig): | ||
| return metrics_cfg.__getstate__() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
register_common_entity_bindingsis 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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used by downstream library, namely Enterprise.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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