Skip to content
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

add flux.conf_builtin.conf_builtin_get() to give Python access to compiled-in config values #6486

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Dec 6, 2024

Currently, Python programs have no way to query libflux for compiled-in config values, but this could be useful if they want to find the current config directory. This could possibly be derived from broker attributes, but in some cases the broker attributes could be set to nothing, so this is not ideal.

Calling in to flux_conf_builtin_get() with FLUX_CONF_AUTO using ffi doesn't work, because the "executable" for Python programs is always going to be a python binary installed in the system (or elsewhere), so the libflux intree detection doesn't work.

This PR introduces a wrapper for flux_conf_builtin_get() for Python that reimplements intree detection using the path to module itself compared to the intree python_path.

Since the motivating use case is to determine the current appropriate Flux confdir from a Python program, this PR also adds a new builtin config key confdir for this purpose.

@grondo grondo force-pushed the python-conf-builtin branch from efe2f32 to 6b36221 Compare December 6, 2024 02:34
Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM!

@grondo grondo force-pushed the python-conf-builtin branch from 6b36221 to 22fd013 Compare December 10, 2024 22:59
@grondo
Copy link
Contributor Author

grondo commented Dec 10, 2024

Thanks! I'll set MWP.

Problem: It would be useful to query the builtin intree or installed
configuration path from libflux, but flux_conf_builtin_get() does
not support that specific value.

Add confdir to the list of supported keys for flux_conf_builtin_get().
Problem: Python programs do not have access to libflux builtin
configuration values via flux_conf_builtin_get().

Add flux.conf_builtin.conf_builtin_get() which provides access to this
function from Python. Unfortunately, the bahavior of FLUX_CONF_AUTO
must be duplicated because the builtin libflux intree vs installed
tests do not work when `python` is the executable (since it isn't
part of Flux).
Problem: There are no unit tests for the Python interface to
flux_conf_builtin_get().

Add some very simple tests.
@grondo grondo force-pushed the python-conf-builtin branch from 22fd013 to 024e5cb Compare December 11, 2024 00:20
@mergify mergify bot merged commit cb2b6f3 into flux-framework:master Dec 11, 2024
35 checks passed
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.61%. Comparing base (7d4d63b) to head (024e5cb).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/bindings/python/flux/conf_builtin.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6486      +/-   ##
==========================================
- Coverage   83.63%   83.61%   -0.02%     
==========================================
  Files         524      525       +1     
  Lines       87654    87680      +26     
==========================================
+ Hits        73309    73318       +9     
- Misses      14345    14362      +17     
Files with missing lines Coverage Δ
src/common/libflux/conf.c 85.56% <ø> (ø)
src/bindings/python/flux/conf_builtin.py 96.15% <96.15%> (ø)

... and 5 files with indirect coverage changes

@grondo grondo deleted the python-conf-builtin branch December 11, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants