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

Fix printing issue in perfstub thirdparty code #3913

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

anagainaru
Copy link
Contributor

During the SC23 ADIOS2 tutorial @khuck noticed that only rank 0 should print the initialization message in perfstub and not all ranks. If this PR is considered an overkill (passing the rank all the way to the initializer) the second option is to just comment out the print.

@anagainaru
Copy link
Contributor Author

The CI errors are unrelated to the changes from what I can tell so the PR can be reviewed

99% tests passed, 2 tests failed out of 2040
2023-11-13T07:41:13.9976528Z 
2023-11-13T07:41:13.9976772Z Total Test time (real) = 4704.15 sec
2023-11-13T07:41:13.9977242Z 
2023-11-13T07:41:13.9977413Z The following tests FAILED:
2023-11-13T07:41:13.9978503Z 	767 - Engine.BP.*/BPChangingShapeWithinStep.MultiBlock/*.BP3.MPI (Timeout)
2023-11-13T07:41:13.9979586Z 	769 - Engine.BP.*/BPChangingShapeWithinStep.MultiBlock/*.BP4.MPI (Timeout)
2023-11-13T07:41:14.0973434Z CMake Error at /__w/ADIOS2/ADIOS2/gha/scripts/dashboard/common.cmake:466 (message):
2023-11-13T07:41:14.0974646Z   Some tests failed

@anagainaru anagainaru merged commit fd111d4 into ornladios:master Nov 13, 2023
31 of 33 checks passed
@khuck
Copy link
Collaborator

khuck commented Nov 13, 2023

LGTM

@caitlinross
Copy link
Collaborator

I'm kind of late to this, but changes should not be made directly to third party libraries like it says in thirdparty/perfstubs/Readme.txt. The changes to timer.h and timer.c should have been made in the actual perfstubs repo, then once merged, run thirdparty/perfstubs/update.sh to do the update for ADIOS. Then you could make the other ADIOS changes in that same PR. I guess with perfstubs it's not a huge deal since I'm assuming that there's not going to be many changes to it? But say there's a bug found in perfstubs in the future that gets fixed and we run the update script to pull in the change, these changes by Ana will possibly be lost (unless someone does make this change in perfstubs).

CC @vicentebolea

@eisenhauer
Copy link
Member

Whoops, @caitlinross is absolutely correct. Oddly, I thought that perfstubs was actually part of ADIOS, despite its location in thirdparty. But this needs to be rolled back, fixed in Kevin's repo and pulled back in from that. (Or, if we don't want to depend upon a third party for this, we need to adopt perfstubs and move it into toolkit.)

@caitlinross
Copy link
Collaborator

Yeah it used to be under toolkit/profiling/taustubs, but a couple of years ago, Chuck had me move it into thirdparty. I don't recall the reasoning for this change, I assume just to make it easier to pull in updates if there are any.

@caitlinross
Copy link
Collaborator

So my vote is not to undo that work that was done a couple of years ago, and leave it as thirdparty and do what @eisenhauer said to rollback, make the change upstream and pull it in correctly.

@khuck
Copy link
Collaborator

khuck commented Nov 27, 2023

yeah, that's my bad - I should have done it in the main repo. I think we should remove it entirely, or make it optional. There's a possibility that PerfStubs could get initialized before MPI_Init, so it wouldn't know the MPI rank. It's also possible to do something like this to get the MPI rank without making an MPI call: https://github.com/UO-OACISS/tau2/blob/df1b6975cf9fd47456793b64277af7af4117cd18/src/Profile/RtsLayer.cpp#L149-L206 ...but it would have to be maintained/updated to support all job schedulers. Maybe the best thing to do is remove the printf entirely.

@caitlinross
Copy link
Collaborator

Yeah I'd probably just remove the printf.

@anagainaru
Copy link
Contributor Author

Ups I didn't know this about thirdparty code :)
I'm ok with rolling this back and removing the printf from the perfstub code

@khuck
Copy link
Collaborator

khuck commented Nov 27, 2023

I have removed the printf in the master branch of perfstubs, you should be good to run the magic script (thirdparty/perfstubs/update.sh) to update ADIOS with the latest version.

@anagainaru
Copy link
Contributor Author

I don't have time to figure out why the update is not working until tomorrow evening so if someone else could help, that would be great. This is what I get:

$ ./thirdparty/perfstubs/update.sh
+ shopt -s dotglob
+ readonly name=perfstubs
+ name=perfstubs
+ readonly 'ownership=PerfStubs Upstream <kwrobot@kitware.com>'
+ ownership='PerfStubs Upstream <kwrobot@kitware.com>'
+ readonly subtree=thirdparty/perfstubs/perfstubs
+ subtree=thirdparty/perfstubs/perfstubs
+ readonly repo=https://github.com/khuck/perfstubs.git
+ repo=https://github.com/khuck/perfstubs.git
+ readonly tag=master
+ tag=master
+ readonly shortlog=true
+ shortlog=true
+ readonly 'paths=
LICENSE
README.md
perfstubs_api/README.md
perfstubs_api/config.h.in
perfstubs_api/timer.h
perfstubs_api/timer.c
perfstubs_api/tool.h
'
+ paths='
LICENSE
README.md
perfstubs_api/README.md
perfstubs_api/config.h.in
perfstubs_api/timer.h
perfstubs_api/timer.c
perfstubs_api/tool.h
'
+ . ./thirdparty/perfstubs/../update-common.sh
++ readonly 'regex_date=20[0-9][0-9]-[0-9][0-9]-[0-9][0-9]'
++ regex_date='20[0-9][0-9]-[0-9][0-9]-[0-9][0-9]'
++ readonly 'basehash_regex=perfstubs 20[0-9][0-9]-[0-9][0-9]-[0-9][0-9] ([0-9a-f]*)'
++ basehash_regex='perfstubs 20[0-9][0-9]-[0-9][0-9]-[0-9][0-9] ([0-9a-f]*)'
++ '[' -n perfstubs ']'
++ '[' -n 'PerfStubs Upstream <kwrobot@kitware.com>' ']'
++ '[' -n thirdparty/perfstubs/perfstubs ']'
++ '[' -n https://github.com/khuck/perfstubs.git ']'
++ '[' -n master ']'
+++ git rev-parse --show-toplevel
++ '[' '!' -d /Users/ana/work/adios/ADIOS2-main/ADIOS2/thirdparty/perfstubs/perfstubs ']'
+++ git rev-list '--author=PerfStubs Upstream <kwrobot@kitware.com>' '--grep=perfstubs 20[0-9][0-9]-[0-9][0-9]-[0-9][0-9] ([0-9a-f]*)' -n 1 HEAD
++ readonly basehash=755737f24811958bf249f9497ead7468b55f685c
++ basehash=755737f24811958bf249f9497ead7468b55f685c
+++ git cat-file commit 755737f24811958bf249f9497ead7468b55f685c
+++ sed -n '/perfstubs 20[0-9][0-9]-[0-9][0-9]-[0-9][0-9] ([0-9a-f]*)/ {s/.*(//;s/)//;p}'
+++ egrep '^[0-9a-f]+$'
sed: 1: "/perfstubs 20[0-9][0-9] ...": extra characters at the end of p command
++ readonly upstream_old_short=
++ upstream_old_short=
++ '[' -n 755737f24811958bf249f9497ead7468b55f685c ']'
++ readonly do_shortlog=true
++ do_shortlog=true
++ readonly workdir=/Users/ana/work/adios/ADIOS2-main/ADIOS2/work
++ workdir=/Users/ana/work/adios/ADIOS2-main/ADIOS2/work
++ readonly upstreamdir=/Users/ana/work/adios/ADIOS2-main/ADIOS2/work/upstream
++ upstreamdir=/Users/ana/work/adios/ADIOS2-main/ADIOS2/work/upstream
++ readonly extractdir=/Users/ana/work/adios/ADIOS2-main/ADIOS2/work/extract
++ extractdir=/Users/ana/work/adios/ADIOS2-main/ADIOS2/work/extract
++ '[' -d /Users/ana/work/adios/ADIOS2-main/ADIOS2/work ']'
++ trap 'rm -rf '\''/Users/ana/work/adios/ADIOS2-main/ADIOS2/work'\''' EXIT
++ git clone https://github.com/khuck/perfstubs.git /Users/ana/work/adios/ADIOS2-main/ADIOS2/work/upstream
Cloning into '/Users/ana/work/adios/ADIOS2-main/ADIOS2/work/upstream'...
remote: Enumerating objects: 898, done.
remote: Counting objects: 100% (78/78), done.
remote: Compressing objects: 100% (61/61), done.
remote: Total 898 (delta 37), reused 37 (delta 16), pack-reused 820
Receiving objects: 100% (898/898), 361.22 KiB | 3.17 MiB/s, done.
Resolving deltas: 100% (565/565), done.
++ '[' -n 755737f24811958bf249f9497ead7468b55f685c ']'
++ git worktree add /Users/ana/work/adios/ADIOS2-main/ADIOS2/work/extract 755737f24811958bf249f9497ead7468b55f685c
Preparing worktree (detached HEAD 755737f24)
fatal: '/Users/ana/work/adios/ADIOS2-main/ADIOS2/work/extract' is a missing but already registered worktree;
use 'add -f' to override, or 'prune' or 'remove' to clear
+ rm -rf /Users/ana/work/adios/ADIOS2-main/ADIOS2/work

@vicentebolea
Copy link
Collaborator

It appears that you have an existing worktree in the path /Users/ana/work/adios/ADIOS2-main/ADIOS2/work

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