Feature/python data sources - #13
Draft
kripnerl wants to merge 4 commits into
Draft
Conversation
New option MAPPING_PLUGIN_PYTHON (default OFF). When ON, the plugin gains lazy embedded-Python data-source and custom-function support, driven by a TOML file pointed at by the MAPPING_PLUGIN_PYTHON_CONFIG env var: [python_data_sources.<name>] module/class/args -- a Python class implementing get(args) -> numpy array [python_custom_functions.<library>] module/functions -- Python callables f(inputs, parameters) -> array If any are declared, init() starts CPython once per uda_server process (xinetd spawns one per connection; all fields served on a connection share the interpreter, the data-source instances and their caches), dlopening libpython RTLD_GLOBAL first because UDA loads plugins RTLD_LOCAL. PYTHONPATH and MAPPING_PLUGIN_PYTHONPATH are folded into sys.path. When nothing is configured the interpreter is never started, so non-Python deployments are unaffected; built with MAPPING_PLUGIN_PYTHON=OFF there is no Python dependency at all and the registration call is a compile-time no-op. The config is separate from the libtokamap config because libtokamap validates it against a compiled-in schema with additionalProperties=false. The PythonDataSource/PythonCustomFunction bridges mirror the conversions in libtokamap's clibtokamap module (DataSource.get -> numpy Array; GIL held for the duration of every Python call), which itself never starts an interpreter — so the plugin is the right place to own the embedded one.
Code review of feature/python-data-sources against
feature/lightweight_plugin_remove_fluff turned up nine defects in the new
embedded-Python support. All of them are fixed here.
Correctness, in rough order of severity:
- After an IMAS_MAP::reset() every later request in the process failed with
"Data source with name 'CDB' already exists": reset() clears the plugin's
m_init without unregistering anything, and the following init() re-ran the
Python registration, which throws on a duplicate name. The registration is
now done at most once per process (m_python_init), and registering
unregisters any previous entry first so a retry after a partially failed
init heals instead of wedging the process.
- _import_array() ran without the GIL whenever CPython had already been
initialised by someone else (the whole Py_Initialize block, which is what
implicitly took the GIL, is skipped on that path). It imports
numpy.core.multiarray and touches CPython objects, so this was a data race
on interpreter state. Now covered by a GilLock on both paths.
- The GIL that Py_Initialize() leaves held was never released: there was no
PyEval_SaveThread(), and the PyGILState_Ensure/Release pair around the
registration returns LOCKED for that same thread so it did not drop it
either. Any Python background thread (a connection-pool reaper, an h5py or
logging worker) could therefore never run while the plugin sat idle, and a
second server thread calling PyGILState_Ensure() would block forever.
MainThreadGilRelease now hands the GIL back on the way out, including when
the initialisation throws.
- A failed TypedDataArray -> NumPy conversion returned nullptr straight into
PyDict_SetItemString, which does Py_INCREF on it: a segfault in uda_server
instead of a DataSourceError. Checked at both call sites; wrap_array_copy
now also checks its malloc and the descr, and json_to_pyobject reports
failure rather than storing nulls.
- MAPPING_PLUGIN_PYTHONPATH was inserted into sys.path as a single string,
never split on ':', so any multi-directory value silently matched nothing.
PYTHONPATH was split, but each entry was inserted at index 0, reversing the
precedence order CPython gives it. Both now go through one
prepend_search_path() helper; MAPPING_PLUGIN_PYTHONPATH still wins.
- Constructor args of non-scalar TOML type were silently dropped, so a data
source configured with a nested table or an array came up with those kwargs
missing and fell back to its defaults. toml_node_to_json now carries tables
and arrays through, dispatching on the node type so numeric types are not
coerced, and errors on dates/times rather than dropping them.
- PyArray_ISCARRAY also requires WRITEABLE, so a read-only but perfectly
contiguous array (np.broadcast_to, an mmap view, a cache deliberately marked
read-only) was rejected as "non-C-contiguous". Uses ISCARRAY_RO — the data is
copied anyway — and the message now names the flag that actually failed.
Build:
- -DMAPPING_PLUGIN_DEFAULT_LIBPYTHON="${Python3_LIBRARIES}" breaks whenever
FindPython3 returns a list: UDA's foreach/add_definitions loop splits it into
two arguments and the compile dies on the unterminated quote. Reproduced with
optimized;<lib>;debug;<lib>. Now picks the single existing python entry.
Also removes six dead lines: an "if (module == nullptr)" block copy-pasted
directly after the identical check that already threw.
Verified: rebuilt the image and ran read/reset/read on one connection — the
third request now reaches the data source (failing only on the placeholder DB
host) where it previously returned "already exists". The interpreter starts,
tokamap_compass imports from MAPPING_PLUGIN_PYTHONPATH and CDBDataSource is
registered, so the sys.path rewrite is exercised. Both files also type-check
clean under -Wall -Wextra against the image's own libtokamap/UDA/NumPy headers,
with and without MAPPING_PLUGIN_PYTHON.
Not addressed: ext_include/toml.hpp is still a byte-identical second copy of
the one in libtokamap, which cannot be resolved from this repo alone —
libtokamap exports nlohmann, valijson, exprtk and inja from ext_include but not
toml.hpp.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ext_include/toml.hpp was a byte-identical second copy of the toml++ 3.4 header already inside libtokamap (17,880 lines, 97% of this feature's diff) supporting roughly 20 lines of config parsing. Two independently-updatable copies of the same header had to be kept in step, and the include search order made it non-obvious which one won. libtokamap installs nlohmann/json.hpp, valijson, exprtk and inja out of its ext_include but NOT toml.hpp, and its own TOML->JSON loader (load_toml_file) is in an anonymous namespace, so there is no way to reach a TOML parser from here without duplicating one. MAPPING_PLUGIN_PYTHON_CONFIG is therefore now read as JSON, with the nlohmann::json libtokamap already exports and this file already used. libtokamap accepts JSON for its own config too, so both files stay in formats libtokamap itself supports. Comments are tolerated (ignore_comments), so the deployed config keeps its explanatory prose; a .toml path that fails to parse says so explicitly rather than leaving the reader guessing. The conversion layer disappears with the parser: config values are already nlohmann::json, which is exactly what json_to_pyobject consumes, so toml_node_to_json is gone. Validation got stricter on the way through — a non-string class_name and a non-object args are now errors instead of being quietly ignored by toml++'s value_or. python_data_source.hpp documents the new config shape, why it is JSON, and — for the libtokamap maintainer — the one-line alternative on their side: adding ext_include/toml.hpp to EXT_HEADERS (libtokamap CMakeLists.txt:136-141) so it is installed like the other four ext headers, or promoting load_toml_file() to public API. Either would let this plugin take TOML configs again with no vendoring. Paired with the uda-compass change that converts config/mapping_plugin_python.toml to .json — the two must land together, since this plugin no longer parses TOML and the previous plugin cannot parse JSON. Verified: 20 assertions against the real parse_python_data_sources / parse_python_custom_functions (deployed shape, class/class_name precedence and default, nested table+array args carried through with types intact, and all eight rejection paths with their messages), plus a rebuilt image running read/reset/read — the CDB source is registered from the JSON config and fails only on the placeholder DB host, with no parse, validation, import or duplicate-registration errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
May be required to plug
tokamapmapping with the Pythonlibtokamap.Mapper. Currently a draft for discussion.Using the TOML configuration: requires ukaea/libtokamap#47 to be accepted first.
AI description follows.
Summary
Add optional support for data sources and custom functions implemented in Python.
MAPPING_PLUGIN_PYTHON, defaulting toOFF;Motivation
Some mapping data sources already exist as Python implementations. Supporting them directly in the plugin avoids maintaining a second C++ implementation or a separate proxy service.
Python remains entirely optional. Builds with
MAPPING_PLUGIN_PYTHON=OFFretain no Python build or runtimedependency.
Configuration
Configuration is supplied separately through
MAPPING_PLUGIN_PYTHON_CONFIG. It cannot be added to the normallibtokamap configuration because that file is validated against a closed schema.
The interpreter starts only when the configuration declares at least one Python data source or custom-function
library.
TOML is the preferred format. JSON remains supported, including
//comments, for backward compatibility. Bothformats are normalized to
nlohmann::jsonbefore the shared validation and registration path.Dependency
TOML support depends on the companion libtokamap PR that exports its bundled
toml.hppheader:The plugin does not vendor its own copy of toml++.
Runtime considerations
Verification
IMAS_MAP::help()plugin initialization;