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

expose flux version in python module #5281

Closed

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Jun 20, 2023

Problem: Python tools that use Flux want custom logic based on the version, and we currently do not easily expose it.
Solution: create a flux.version "attribute" that calls into a module to call flux_core_version_string().

@@ -0,0 +1,16 @@
###############################################################
# Copyright 2014 Lawrence Livermore National Security, LLC
Copy link
Member

Choose a reason for hiding this comment

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

2023 :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

lol! @chu11 maybe I am living in the past... or maybe I just copy pasted from handle.py 😆

@chu11
Copy link
Member

chu11 commented Jun 20, 2023

One random idea based on the slack conversation. If we can figure out why FLUX_CORE_VERSION_STRING is not being put into flux.constants, we could change this to just return flux.constants.FLUX_CORE_VERSION_STRING.

Granted flux_core_version_string() just returns that too, so saving that one function call is not much value. This is more about "why isn't FLUX_CORE_VERSION_STRING in flux.constants".

Problem: Python tools that use Flux want custom logic based on the version,
and we currently do not easily expose it.
Solution: create a flux.__version__ "attribute" that calls into a module
to call flux_core_version_string().

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Member Author

vsoch commented Jun 20, 2023

Granted flux_core_version_string() just returns that too, so saving that one function call is not much value. This is more about "why isn't FLUX_CORE_VERSION_STRING in flux.constants".

We could definitely look into that, but I've done refactors of the Python bindings so many times, looking at the older version gives me the shivers. If someone has a hint / direction for where to look I can help, but I think my instinct right now is to run away! For context, we now have:

We can do changes in the older one, but just need to be careful to pass them forward to the more hardened one. Also, the separate bindings can easily ship with __version__ if we wanted, because they are built with the exact version of Flux (it happens in the CI). That said, I haven't gotten much feedback about the external bindings so I'm not really sure people are interested.

from flux.core.inner import raw


def __getattr__(name):

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns Note

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
@vsoch
Copy link
Member Author

vsoch commented Jun 20, 2023

Weird error:

Warning:  One or more build-args [BASE_IMAGE FLUX_SECURITY_VERSION GID IMAGESRC] were not consumed

@grondo
Copy link
Contributor

grondo commented Jun 20, 2023

If we can figure out why FLUX_CORE_VERSION_STRING is not being put into flux.constants,

I think the problem is that enumerated constants are the only thing put into flux.constants, all the #define constants are dropped. I know nothing about cffi, so I'm not sure if this is because cffi just can't handle defined constants or... @trws would probably know.

That is I presume why we have this in _core_preproc.py:

#define FLUX_JOBID_ANY 0xFFFFFFFFFFFFFFFF

@trws
Copy link
Member

trws commented Jun 20, 2023 via email

@chu11
Copy link
Member

chu11 commented Jun 20, 2023

We may be filtering them out. CFFI does actually support defines, but only simple ones, and I may have just gone with avoiding the problem entirely rather than trying to pull in the ones that work.

I initially thought this might have been a dumb thing, like we weren't including version.h somewhere. But since this may be a little trickier, I'll write up an issue as a TODO for later on.

@vsoch
Copy link
Member Author

vsoch commented Jun 20, 2023

That sounds great, thanks @chu11 ! I directed the maestro developer how to get the version via an attribute, which should work for now. I also think they might just be calling flux --version to the command line, and the actual issue was being able to parse a git version (with the hasheedoo).

@garlick
Copy link
Member

garlick commented May 16, 2024

Since we opened #5285 to follow this up and the user has a workaround, it looks like the PR can be closed.

@garlick garlick closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants