Feature #3065 benchmarking ensemble stat #3137

bikegeek · 2025-04-24T00:22:56Z

Expected Differences

Do these changes introduce new tools, command line arguments, or configuration file options? [Yes]

If yes, please describe:
Benchmarking using the CTRACK tool that runs when the --enable-profiler option is included in the configuration step of a MET build.

Instrumented code:

src/basic/vx_util/main.cc
src/tools/core/ensemble_stat/ensemble_stat.cc
src/tools/core/ensemble_stat/ensemble_stat_conf.cc

Configuration:

benchmark.yaml:

for exercising MET code via command line options
for exercising MET code via METplus use case(s) (i.e. wrapper code)
Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]

Pull Request Testing

Describe testing already performed for these changes:

on host 'seneca':

cloned MET code to:
/d1/projects/Benchmark_EnsembleStat

built code without and with --enable-profiler to verify the option is working as expected
ran profiling/benchmarking tool, CTRACK on ensemble stat code via:
- MET command line commands:
- use the benchmark.yaml config file set up for running MET command
- use the setup.bash script to set the appropriate environment variables
- use the envs_for_met.bash for setting the environment variables needed to run MET command line command
- METplus use case
- set the settings for METplus in the benchmark.yaml config file

Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

- verify that instructions under the Contributor's Guide are correct and make sense
- only focus on testing MET command line tools
  use the benchmark.yaml config file
  use the setup.bash script to set the appropriate environment variables
  use the envs_for_met.bash  for setting the environment variables needed to run MET command line command
  verify that results are in the output directory specified in the benchmark.yaml config file

Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes ]
Do these changes include sufficient testing updates? [No]
Will this PR result in changes to the MET test suite? [No]
Will this PR result in changes to existing METplus Use Cases? [No]

If yes, create a new Update Truth METplus issue to describe them.
Do these changes introduce new SonarQube findings? [Yes]

If yes, please describe:
may result in lower code coverage percentage for new code
Please complete this pull request review by for RC1 release.

Pull Request Checklist

See the METplus Workflow for details.

Review the source issue metadata (required labels, projects, and milestone).
Complete the PR definition above.
Ensure the PR title matches the feature or bugfix branch name.
Define the PR metadata, as permissions allow.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: METplus-X.Y Support project for bugfix releases or MET-X.Y Development project for the next coordinated release
After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
Close the linked issue and delete your feature or bugfix branch from GitHub.

* Per #3020, add missing GridStatNcOutInfo::do_seeps flag and use it to determine if SEEPS information should be written to the Grid-Stat NetCDF matched pairs output file. * Unrelated to #3020, fix broken NetCDF cf-conventions links in the User's Guide. * Per #3020, no real changes. Just whitespace

… GridStatConfig_SEEPS config file needs to be updated with nc_pairs_flag.seeps = TRUE in order for the same output to be produced by the unit tests.

* Per #3032, add data type column to all of the output tables * Per #3032, remove the first row from each output table since its info is repeated from the table name. Additional changes for consistency and accuracy in column names. * Update docs/Users_Guide/gsi-tools.rst Co-authored-by: Julie Prestopnik <jpresto@ucar.edu> --------- Co-authored-by: Julie Prestopnik <jpresto@ucar.edu>

…o create and push an updated test output image.

* Per #3033, update version info, consolidate release notes, and add upgrade instructions. * Per #3033, remove all instances of 'Bugfix: ' from the release notes since it's redundant with the dropdown name * Per #3030, based on request from Randy Pierce, also add MTD header columns to met_header_columns_v12.0.txt to make it easier to parse the output from MET. * Per #3033, fix typo and correct alignment in table

Removing reference to beta version

Remove references to beta version

Update paths for eckit and atlas

Remove beta references

…cases

…acro enables

… setting in the yaml config file

…er use case config files, add more assertion checks

…ve to MET_BASE (<install_loc>/share/met) and other files that are only in the MET repo are found relative to MET_TEST_BASE (MET/internal/test_unit). Also remove MET_BUILD_BASE env var (#3052)

… is expected.

…after finishing, format the information file

…re saved.

…e_info()

davidalbo · 2025-05-01T17:44:36Z

Assuming this is a first example of profiling to come, I'm wondering about putting the profiling in so many places in ensemble_stat. Not that I'm against that but is there a good reason for it? In other words, what is the intent of profiling this particular application, and does the way it is set up help accomplish that?

bikegeek · 2025-05-01T18:24:34Z

I was taking a guess at what functions might be of interest to instrument since I'm not familiar with the code. If you don't think all these functions should be instrumented, feel free to undo or let me know which ones to undo.

…

On Thu, May 1, 2025 at 11:44 AM davidalbo ***@***.***> wrote: *davidalbo* left a comment (dtcenter/MET#3137) <#3137 (comment)> Assuming this is a first example of profiling to come, I'm wondering about putting the profiling in so many places in ensemble_stat. Not that I'm against that but is there a good reason for it? In other words, what is the intent of profiling this particular application, and does the way it is set up help accomplish that? — Reply to this email directly, view it on GitHub <#3137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4UJHUWOX3226CE2CQU4H324JMRTAVCNFSM6AAAAAB3XULGE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBVGM2DKNBRHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

davidalbo · 2025-05-05T16:15:06Z

Thinking out loud, I'd describe my thinking like this: Start at a high level, look for bottlenecks, and drill down from there, meaning put only a few ctrack profiling lines initially and see where the app is spending all it's time, then look deeper. If you put profiling everywhere it might be harder to see where to investigate/modify. If that makes sense, I can give it a go as far as making it look like what I think would be a good initial setup. If that does NOT make sense, that is also ok. Let me know what you think.

bikegeek · 2025-05-05T16:16:47Z

That sounds good. Thanks Dave!

…

On Mon, May 5, 2025 at 10:15 AM davidalbo ***@***.***> wrote: *davidalbo* left a comment (dtcenter/MET#3137) <#3137 (comment)> Thinking out loud, I'd describe my thinking like this: Start at a high level, look for bottlenecks, and drill down from there, meaning put only a few ctrack profiling lines initially and see where the app is spending all it's time, then look deeper. If you put profiling everywhere it might be harder to see where to investigate/modify. If that makes sense, I can give it a go as far as making it look like what I think would be a good initial setup. If that does NOT make sense, that is also ok. Let me know what you think. — Reply to this email directly, view it on GitHub <#3137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4UJHW4VOXIHECICXC2P3L246FB7AVCNFSM6AAAAAB3XULGE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNJRGUYTSMBWGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

davidalbo · 2025-05-05T16:42:47Z

@bikegeek was there a command line option to turn on the profiling? I'd like to test out my changes.

bikegeek · 2025-05-05T16:46:06Z

Contributor's Guide: https://metplus.readthedocs.io/projects/met/en/feature_3065_benchmarking_ensemble_stat/Contributors_Guide/code_profiling.html Section 7.1.3.1 Step 2 Compile MET Code

…

On Mon, May 5, 2025 at 10:43 AM davidalbo ***@***.***> wrote: *davidalbo* left a comment (dtcenter/MET#3137) <#3137 (comment)> @bikegeek <https://github.com/bikegeek> was there a command line option to turn on the profiling? I'd like to test out my changes. — Reply to this email directly, view it on GitHub <#3137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4UJHRPNWBA46VCTKF5XX3246IJ3AVCNFSM6AAAAAB3XULGE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNJRGYYDKNZWHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

davidalbo · 2025-05-05T17:14:09Z

Hopefully the last question. Is there a way to look at these files in a way that formats them nicely? Just treating them as ascii gives output that is hard to read.

detail_output.txt
summary_output.txt

bikegeek · 2025-05-05T17:53:35Z

The metrics files are consolidated by the Python code and stored in the directory as indicated in the YAML config file: *benchmark_output_path *setting, then if you specified the filename, the final file is <filename>_timestamp.csv, <filename>_timestamp.txt. If no filename is specified in the filename setting, then the consolidated files are timestamp.csv, timestamp.txt. *You can then view the csv file via spreadsheet. *I prefer to view using jupyter lab and opening up into a dataframe view. Here is the example config file: https://metplus.readthedocs.io/projects/met/en/feature_3065_benchmarking_ensemble_stat/Contributors_Guide/code_profiling.html Expand the drop down: section 7.1.3.1 under the Edit the benchmark.yaml configuration file

…

On Mon, May 5, 2025 at 11:14 AM davidalbo ***@***.***> wrote: *davidalbo* left a comment (dtcenter/MET#3137) <#3137 (comment)> Hopefully the last question. Is there a way to look at these files in a way that formats them nicely? Just treating them as ascii gives output that is hard to read. detail_output.txt summary_output.txt — Reply to this email directly, view it on GitHub <#3137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4UJHQJMJROWO6JEMHD3ML246L7PAVCNFSM6AAAAAB3XULGE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNJRGY4TMMJWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

davidalbo · 2025-05-05T19:20:12Z

I reduced the profiling to the two main function calls only. process_n_vld and process_vx. This shows that process_vx is where the bulk of the time is being spent, so that is where one might add more profiling to see where within that method it is getting bogged down. This is what I was after. If you're good with this, I'll commit and push my changes.

bikegeek · 2025-05-05T19:24:12Z

Sounds good, thanks Dave!

…

On Mon, May 5, 2025 at 1:20 PM davidalbo ***@***.***> wrote: *davidalbo* left a comment (dtcenter/MET#3137) <#3137 (comment)> I reduced the profiling to the two main function calls only. process_n_vld and process_vx. This shows that process_vx is where the bulk of the time is being spent, so that is where one might add more profiling to see where within that method it is getting bogged down. This is what I was after. If you're good with this, I'll commit and push my changes. Screenshot.2025-05-05.at.1.17.23.PM.png (view on web) <https://github.com/user-attachments/assets/eb9d3ac0-1ca7-4e92-adae-e75b9a6847b4> — Reply to this email directly, view it on GitHub <#3137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4UJHQLT2PVIHQ6ODTO4632462YHAVCNFSM6AAAAAB3XULGE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNJSGA4TGNZVGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…l methods

davidalbo

I approve this pull request.

docs/Contributors_Guide/code_profiling.rst

JohnHalleyGotway · 2025-05-08T22:11:20Z

src/tools/core/ensemble_stat/ensemble_stat.cc

@@ -194,6 +198,11 @@ int met_main(int argc, char *argv[]) {
   // Perform verification
   process_vx();

+  // Save the CTRACK metrics 
+  #ifdef WITH_PROFILER
+  ctrack::result_print();


@bikegeek if ctrack::result_print() is already being called every time in basic/vx_util/main.cc don't we NOT need to call it in the application code? Won't this cause the results to be printed twice?

If ctrack::result_print() is NOT in the application code, the benchmarking information for the application code does not get saved.

JohnHalleyGotway

@bikegeek, apologies for the long delay in getting to this. I proposed a couple of minor edits in the docs.

I tested in seneca as the met_test user in /d1/projects/MET/MET_pull_requests/met-12.1.0/rc1/MET-feature_3065_benchmarking_ensemble_stat and compiled with --enable-profiler.

When I run make test, I do see all the tests being run, including CTRACK output, and that's good. But it fails with this error at the end:

make[1]: *** No rule to make target 'profiler', needed by 'all'.  Stop.
make[1]: Leaving directory '/d1/projects/MET/MET_pull_requests/met-12.1.0/rc1/MET-feature_3065_benchmarking_ensemble_stat/scripts'
make: *** [Makefile:880: test] Error 2

I'll see if I can figure out why that's happening.

…ining which tests to run.

JohnHalleyGotway

OK, I just pushed a fix to the make test error. It's because of custom logic in scripts/Makefile to determine which test scripts to run. But I patched it to ignore the --enable-profiler configuration setting.

I approve of this PR, but do recommend that you make those documentation tweaks.

I imagine as we start using this profiling option, we may want to modify the details. @georgemccabe mentioned one idea to push the CTRACK directives in a common library function, so that adding it to a function is a one-liner instead of a three-liner.

Thanks!

No more error message during make test. Update comiler section accordingly

Co-authored-by: John Halley Gotway <johnhg@ucar.edu>

bikegeek · 2025-05-13T21:30:48Z

@davidalbo / @JohnHalleyGotway

I updated the documentation and just need one of you to click 'approve'

JohnHalleyGotway

I approve of these changes.

I'm sorry I missed the ping on this yesterday!

georgemccabe and others added 30 commits November 15, 2024 06:17

use custom GitHub Action to trigger METplus use cases

30bf4fb

Updating values

8755d8d

Update to reflect usage of oneAPI compilers

850e66c

Updating file to reflect usage of oneAPI compilers

46784f1

Hotfix to the main_v12.0 branch after PR #3022 fixed a SEEPS bug. The…

3d3f5f5

… GridStatConfig_SEEPS config file needs to be updated with nc_pairs_flag.seeps = TRUE in order for the same output to be produced by the unit tests.

Adding In Memoriam

171535f

Making a superficial change in the main_v12.0 branch to trigger GHA t…

580b0b1

…o create and push an updated test output image.

Update install_met_env.acorn

b3f697b

Removing reference to beta version

Update install_met_env.cactus

bfc1a76

Remove references to beta version

Update install_met_env.cactus

7cc75a8

Update paths for eckit and atlas

Update install_met_env.wcoss2

8ed8481

Remove beta references

initial YAML config file used to perform benchmarking on METplus use …

1b6296f

…cases

initial version of code used to run use case on MET that has CTRACK m…

60ebeee

…acro enables

Fix typo, missing one * to make SciPy bold in appendixF.rst

19de85d

Add support to run the use case multiple times, based on the num_runs…

b6d2066

… setting in the yaml config file

Add support for determining the path where the YAML file is located

9445c35

Allow a list of wrapper config files, create subdirectories named aft…

446b8c3

…er use case config files, add more assertion checks

Add list to wrapper config file

1ec71d9

Per #3051, update unit tests so that installed files are found relati…

958d47c

…ve to MET_BASE (<install_loc>/share/met) and other files that are only in the MET repo are found relative to MET_TEST_BASE (MET/internal/test_unit). Also remove MET_BUILD_BASE env var (#3052)

Removed incorrectly placed asserts and fixed incorrect comment

5758fcb

Fixed assertion that checks output file size

122ebe4

Remove assert in check, checking for a single config file when a list…

6b9190e

… is expected.

clean up comments, mv summary_output.txt and detail_output.txt files …

155c81a

…after finishing, format the information file

Removed option to save output as csv, tabular or both. Both formats a…

8f2e1e0

…re saved.

Fixed name of config file

e170280

Fix the command to get the directory where the script has been invoked

893759e

Incorrectly passing in the use case path instead of name into generat…

0efd9fd

…e_info()

JohnHalleyGotway changed the title ~~Feature 3065 benchmarking ensemble stat~~ Feature #3065 benchmarking ensemble stat May 1, 2025

Reduced the number of profiling points to just the two major top leve…

b1f7b9e

…l methods

davidalbo previously approved these changes May 5, 2025

View reviewed changes

JohnHalleyGotway reviewed May 8, 2025

View reviewed changes

docs/Contributors_Guide/code_profiling.rst Outdated Show resolved Hide resolved

JohnHalleyGotway reviewed May 8, 2025

View reviewed changes

docs/Contributors_Guide/code_profiling.rst Outdated Show resolved Hide resolved

JohnHalleyGotway reviewed May 8, 2025

View reviewed changes

Per #3065, fix scripts/Makefile to ignore ENABLE_PROFILER when determ…

8f75787

…ining which tests to run.

JohnHalleyGotway dismissed davidalbo’s stale review via 8f75787 May 8, 2025 22:34

JohnHalleyGotway previously approved these changes May 8, 2025

View reviewed changes

Update code_profiling.rst

95e6008

No more error message during make test. Update comiler section accordingly

bikegeek dismissed JohnHalleyGotway’s stale review via 95e6008 May 13, 2025 17:53

bikegeek and others added 2 commits May 13, 2025 12:46

Update docs/Contributors_Guide/code_profiling.rst

62f42b8

Co-authored-by: John Halley Gotway <johnhg@ucar.edu>

Update docs/Contributors_Guide/code_profiling.rst

13a3306

Co-authored-by: John Halley Gotway <johnhg@ucar.edu>

bikegeek requested a review from JohnHalleyGotway May 13, 2025 18:48

JohnHalleyGotway approved these changes May 14, 2025

View reviewed changes

bikegeek merged commit e0c0573 into develop May 14, 2025
29 checks passed

github-project-automation bot moved this from 🔎 In review to 🏁 Done in METplus-6.1 Development May 14, 2025

bikegeek deleted the feature_3065_benchmarking_ensemble_stat branch May 14, 2025 19:58

Feature #3065 benchmarking ensemble stat #3137

Feature #3065 benchmarking ensemble stat #3137

Uh oh!

Conversation

bikegeek commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Expected Differences

Pull Request Testing

Pull Request Checklist

Uh oh!

davidalbo commented May 1, 2025

Uh oh!

bikegeek commented May 1, 2025 via email

Uh oh!

davidalbo commented May 5, 2025

Uh oh!

bikegeek commented May 5, 2025 via email

Uh oh!

davidalbo commented May 5, 2025

Uh oh!

bikegeek commented May 5, 2025 via email

Uh oh!

davidalbo commented May 5, 2025

Uh oh!

bikegeek commented May 5, 2025 via email

Uh oh!

davidalbo commented May 5, 2025

Uh oh!

bikegeek commented May 5, 2025 via email

Uh oh!

davidalbo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JohnHalleyGotway May 8, 2025

Choose a reason for hiding this comment

Uh oh!

bikegeek May 13, 2025

Choose a reason for hiding this comment

Uh oh!

JohnHalleyGotway left a comment

Choose a reason for hiding this comment

Uh oh!

JohnHalleyGotway left a comment

Choose a reason for hiding this comment

Uh oh!

bikegeek commented May 13, 2025

Uh oh!

JohnHalleyGotway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bikegeek commented Apr 24, 2025 •

edited

Loading