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

Update LDMS Variorum sampler to support Variorum v0.8 #1447

Closed
wants to merge 9 commits into from

Conversation

tpatki
Copy link
Contributor

@tpatki tpatki commented Sep 18, 2024

Work in Progress, not ready for review.

Fixes #1431.

This PR updates the JSON API and the parsing of power data based on Variorum 0.8.

ToDo:

  • Code Formatting
  • Documentation updates
  • Squash commits
  • Autoconf checks
  • Testing (see below)

Testing:

  • IBM Alehouse node, CPU+GPU build
  • IBM Alehouse node, GPU-build only
  • IBM Alehouse node, CPU-build only
  • Other LLNL systems with @kshoga1 and @slabasan.

@tpatki tpatki marked this pull request as draft September 18, 2024 21:38
@morrone
Copy link
Collaborator

morrone commented Sep 19, 2024

@tpatki Could you also please this squash this PR together into just a small set of clean commits?

You will also need to include appropriate autoconf checks since this will now only work with variorum 0.8 or newer.

@tpatki
Copy link
Contributor Author

tpatki commented Sep 20, 2024

@morrone
Thanks for the quick feedback! :)

Yes, I will squash into clean commits when we're done testing and when I mark this as ready for review (will be a few weeks until @kshoga1 can test on Mutt.)

You will also need to include appropriate autoconf checks since this will now only work with variorum 0.8 or newer.

Could you please clarify this? We were not checking for a specific version in configure.ac here earlier. Where/how should we include the autoconf checks (is there an example we can follow)?

@morrone
Copy link
Collaborator

morrone commented Sep 23, 2024

Could you please clarify this? We were not checking for a specific version in configure.ac here earlier. Where/how should we include the autoconf checks (is there an example we can follow)?

Better than a specific version check is to test for features in the library that the code requires. For instance, are there some unique new function in the new version of the library? You could add a test for that function in the library.

LDMS's configure.ac has an AC_LIB_HAVE_LINKFLAGS test for variorum. There is an additional "testcode" field for that macro where you could put a tiny bit of C code that calls a function that is unique to the newer version of the code.

https://www.gnu.org/software/gnulib/manual/html_node/Searching-for-Libraries.html

@tom95858
Copy link
Collaborator

tom95858 commented Oct 9, 2024

@tpatki, @morrone what is the status of this PR?

@tpatki
Copy link
Contributor Author

tpatki commented Oct 9, 2024

Hi @tom95858 : This is draft PR at the moment. Kathleen (@kshoga1) will need to test this at scale internally. We plan to do that in this month. Once we have tested at scale, we'll address Chris' comments, update it as ready for review, and it can be merged.

@tom95858
Copy link
Collaborator

tom95858 commented Oct 9, 2024

@tpatki Ok great, thanks for the update.

@tom95858
Copy link
Collaborator

@morrone, please confirm that this is not a target for 4.4.5.

@morrone
Copy link
Collaborator

morrone commented Oct 22, 2024

@morrone, please confirm that this is not a target for 4.4.5.

Except for reviewing here, I'm not really involved in that work. But my understanding is that it'll happen when it happens, and we should not hold up the release for this work.

@tom95858
Copy link
Collaborator

tom95858 commented Nov 1, 2024

@tpatki please reopen this pull request when it is ready for review.

@tom95858 tom95858 closed this Nov 1, 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.

Update Variorum sampler to support v0.8
3 participants