feat: Make interfaces shared libraries, add ArgReader utility to serve as a demonstration#171
Conversation
…(0 and 1 values accepted)
…the qmain/main shim, allowing utilities to depend on the interface as a shared library
…interfaces, not libselene itself
… base rather than to something platform specific. Move to dynamic libs rather than static libs to achieve support for multiple utilities
…res an existing defintion
…binding specifically to helios_qis. Correct rpath of helios_qis for installs so that base_qis can be found.
| "HeliosObjectFileToSeleneObjectFileStep_Linux", | ||
| "HeliosObjectFileToSeleneExecutableStep_Windows", | ||
| "HeliosObjectFileToSeleneExecutableStep_Darwin", | ||
| "HeliosObjectFileToSeleneExecutableStep", |
There was a problem hiding this comment.
Note: we no longer require platform-specific branching in the build process
| link_flags=plugin.link_flags, | ||
| library_search_dirs=plugin.library_search_dirs, | ||
| ) | ||
| def from_plugin(cls, plugin) -> list[Self]: |
There was a problem hiding this comment.
A plugin can now have dependencies on other plugins, e.g. the utility plugin can depend on a specific interface (such as base QIS). To allow for this, we now return a list from from_plugin - note that this is only used internally.
There was a problem hiding this comment.
Pull request overview
Refactors selene's QIS interfaces from static into shared libraries to enable composing multiple utility plugins, splits HeliosQIS into a shared shim plus a thin static runner, and adds a new ArgReader utility (plus a selene_log_utility_call FFI hook so utilities can record their own activity in Selene traces).
Changes:
- Introduce a BaseQIS shared library and reshape HeliosQIS to depend on it, updating CMake/Hatch build glue for
.so/.dylib/.dll.a. - Add
selene_log_utility_callFFI +Emulator::log_custom_callso utilities can emit Custom trace events. - Add the
argreaderutility: a Rust shim, a PythonArgProvidercontext manager that writes a YAML file and exportsSELENE_ARGREADER_INPUT_FILE, and a plugin wiring it into builds.
Reviewed changes
Copilot reviewed 64 out of 67 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| selene-sim/rust/ffi_interface.rs | Adds selene_log_utility_call FFI entrypoint with doc comment. |
| selene-sim/rust/emulator.rs | Adds log_custom_call; moves on_user_call to before custom_call invocation. |
| selene-sim/c/include/selene/selene.h | Regenerated header with include guard and new selene_log_utility_call declaration. |
| selene-sim/cbindgen.toml | Enables SELENE_SIM_H include guard. |
| selene-sim/c/CMakeLists.txt | Picks the correct platform-specific selene library filename. |
| selene-sim/python/selene_sim/build.py | Adapts to LibDep.from_plugin returning a list. |
| selene-ext/interfaces/base_qis/* | New shared BaseQIS interface (C sources, headers, plugin). |
| selene-ext/interfaces/base_qis/c/src/print.c | Print helpers — contains a stray debug fprintf. |
| selene-ext/interfaces/helios_qis/* | Helios reshaped as shared shim + static runner depending on BaseQIS. |
| selene-ext/utilities/argreader/Cargo.toml | New crate; declares several unused dependencies. |
| selene-ext/utilities/argreader/rust/lib.rs | Rust shim implementing argreader_get_* functions and YAML parsing. |
| selene-ext/utilities/argreader/python/selene_argreader_plugin/* | Plugin and ArgProvider context manager (temp-file/env-var lifecycle issues). |
Comments suppressed due to low confidence (4)
selene-ext/utilities/argreader/python/selene_argreader_plugin/provider.py:64
del os.environ["SELENE_ARGREADER_INPUT_FILE"]raisesKeyErrorif the variable is missing (e.g., another component cleared it, or__enter__was never reached because of an earlier exception). Useos.environ.pop("SELENE_ARGREADER_INPUT_FILE", None)instead, and place the cleanup logic so that the temp file is removed even when__exit__runs due to an exception.
selene-ext/utilities/argreader/python/selene_argreader_plugin/provider.py:55yaml.dump(self.run_inputs, f)serialises a PythonRunInputsdataclass instance using PyYAML's default representer, which produces Python-specific tags such as!!python/object:selene_argreader_plugin.provider.RunInputs. The Rust side parses this file withserde_yml::from_reader::<RunInputs>(reader), which expects plain YAML mappings (shot_inputs: [...], etc.) and will fail to deserialize the Python-tagged form. Convert the dataclasses to plain dicts/lists (e.g. viadataclasses.asdict) and useyaml.safe_dumpbefore writing.
selene-ext/utilities/argreader/python/selene_argreader_plugin/provider.py:64- The
__exit__method only deletes the environment variable; it never propagates exceptions or returns a value. Pyright/mypy treat the implicit-> Nonereturn as "do not suppress", which is what you want — but the temp file is also never cleaned up here (see related comment). The bigger issue is that the temporary YAML file written in__enter__is left on disk forever; on CI this can accumulate quickly.
selene-ext/utilities/argreader/python/selene_argreader_plugin/provider.py:64 - Setting
os.environinside__enter__is process-wide and not thread-safe. If twoArgProvidercontexts overlap (e.g. nested or running in parallel test threads), the second__enter__clobbers the first's env var, and the first__exit__will delete the var while the second context still needs it — leading to confusing test failures. Consider documenting this restriction or providing a more local way of passing the path into the user program.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 67 changed files in this pull request and generated 20 comments.
Comments suppressed due to low confidence (6)
selene-ext/utilities/argreader/rust/lib.rs:415
get_key(key_ptr)is called a second time here, shadowing thekeyvariable defined at line 411. This is redundant work (an additional unsafe pointer read + UTF-8 decode + allocation) and the result is unused other than for the panic message, which would already be identical. Remove this line.
selene-ext/utilities/argreader/rust/lib.rs:179value_helperclones the fullShotInputs(including all records) every time any argument is fetched — once into the cache and again as the return value. For a shot with many records or large array values, this isO(records + total_array_bytes)per fetch. SinceINPUTSis aRefCelland not concurrently accessed, returning a borrow (or at least cloning only the single requested record) would be much more efficient. The current implementation also clones the entire shot a third time (shot_inputs.clone()inside the*cache_cell.borrow_mut() = Some(...)assignment), then returns the originalshot_inputswhich is itself anunwrap_or_elseclone.
selene-ext/utilities/argreader/rust/lib.rs:76CString::new(message)will panic ifmessagecontains an interior NUL byte. Sincemessageis built fromformat!(...)with user-controlled key names and YAML-parsed values that could include NUL bytes (e.g. an attacker-controlled YAML file, or any binary in a string field), this can cause a Rust panic that bypasses the controlledselene_panicpath. Consider replacing NUL bytes with e.g.?before constructing the CString, or usingCString::from_vec_with_nul_unchecked-style replacement.
selene-ext/utilities/argreader/rust/lib.rs:132get_keyreads one byte as length and thenstd::slice::from_raw_parts(string_start, length as usize). There's no length cap enforced here matching the documented 255-char maximum, and more importantly there is no way for this function to validate that the caller actually allocatedlengthbytes after the pointer — which is an unsafe-API constraint, but worth a comment. Also, when the UTF-8 decode fails, the error message does not include any hint (e.g. the failing bytes) that might help users diagnose. The CL-string format (length byte + bytes) means a non-UTF-8 sequence is plausible and worth surfacing.
selene-ext/utilities/argreader/rust/lib.rs:398- In
argreader_get_u64_array, theI64ArrayandF64Arraybranches first build the full convertedu64_valuesvector and only then checku64_values.len() != len. If the user provides an array of wrong length, work is wasted (and panic message usesvalues:?for the original array, which is good). More importantly: the length check should happen before the negative/non-integral element checks too, so that the panic about negative values isn't emitted when the underlying issue is the wrong array length. Also,len: u64is cast tousize— on 32-bit targets a sufficiently largelentruncates silently. Validatelen <= usize::MAX as u64first. The same pattern appears inargreader_get_i64_arrayandargreader_get_f64_array.
selene-ext/utilities/.gitignore:1 - This file is being deleted. The previous
!.gitignoreline was presumably used in combination with a parent.gitignorerule that excluded the wholeutilities/directory (so that the.gitignoreitself remained tracked, allowing files to be added later). With the file removed, please verify no parent.gitignorestill excludesselene-ext/utilities/, otherwise new files added underutilities/argreader/will be silently ignored by git.
Agent-Logs-Url: https://github.com/Quantinuum/selene/sessions/7a2d5a84-6177-4e16-920f-6d68993c85ba Co-authored-by: jake-arkinstall <65358059+jake-arkinstall@users.noreply.github.com>
|
As a final check, I have tested running this change (static interface -> dynamic interface) with pre-existing static utilities on all operating systems, just in case there was a linking issue on any of them - all worked fine. |
…Base QIS interface that is missing, not the Helios QIS interface
Agent-Logs-Url: https://github.com/Quantinuum/selene/sessions/f306b326-0f48-40d4-959e-2a032122b024 Co-authored-by: jake-arkinstall <65358059+jake-arkinstall@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Quantinuum/selene/sessions/f68269cb-70d3-4a2c-ad92-9658c1aee89d Co-authored-by: jake-arkinstall <65358059+jake-arkinstall@users.noreply.github.com>
…AGNOSTIC underscores in interface.c
…error if no arguments are provided to the arg reader
Replaces the static QIS interface shim with a shared interface such that utilities may bind to it cleanly. Adds an argument reader utility, using the new dynamic interface, enabling user program calls to fetch arguments provided at runtime. (Long description modified to satisfy conventional commits)
Presently, the QIS interface shim for selene is a static library, and utilities are expected to be static libraries that depend on the definitions exported by it. This is problematic because, for the purpose of rust-written utilities, one cannot neatly link multiple staticlibs together, causing issues when running multiple utilities. It also makes for a less clear interface.
As such, this PR moves to using a dynamic library for the QIS interface. Specifically:
This opens up the opportunity to use multiple utilities at a time. We also add the ability for utilities to register their actions within selene Trace dumps, including arbitrary data.
ArgReader
Motivating this change, I add the first public utility into the Selene repository: ArgReader, which can be used to provide runtime parameters into the user program.
When building using the ArgReaderPlugin, user code gains access to the following functions:
Note that the tag strings follow the same format as result() and panic() in that they start with a length byte, must be utf-8, and are not required to be null-terminated. As such, their maximum length is 255 characters. They do not require namespacing (e.g. "USER:INT:" in the result stream), but this may be added as long as the arguments are provided with the same namespacing. Error handling is handled internally and will result in descriptive panics.
The user then runs selene within a context manager:
which writes parameters to a temporary YAML file, then sets the environment variable
SELENE_ARGREADER_INPUT_FILEto point to this file. When the user program is running and a parameter is read for the first time, this yaml file is parsed with Serde and stored in memory. Arguments are extracted based on the current shot.ArgReader utilises the new ability to export utility calls to the selene Trace, to provide each parameter's type and value upon read.
Future Work
Now that the QIS interface is dynamic, we can make use of CFFI to make use of interactive selene with different QIS interfaces, rather than pinning directly to selene sim's exposed functions.