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 class name to region #162

Merged
merged 5 commits into from
Dec 4, 2023
Merged

Add class name to region #162

merged 5 commits into from
Dec 4, 2023

Conversation

bertwesarg
Copy link
Member

Closes #161

@elwer please give this a try. @Flamefire maybe you have also the chance to look over this?

@bertwesarg bertwesarg force-pushed the 161-fix branch 3 times, most recently from 2e5b966 to 75ee8ad Compare November 21, 2023 06:49
@AndreasGocht
Copy link
Collaborator

Hey,

just one remark: I choose to use the package name (before first .) as function group, as traces otherwise get to colorful and confusing in vampir. I found it useful for a first analysis to identify only used packages. I think the ideal solution would be, if Score-P/Vampir would support a hirachcy of groups. However it might also be a solution to add an option to manage the spitting. Or you could simply ignore my remark 😉.

And just out of curiosity: do you need to call std::move explicitly these days?

Best Andreas

@bertwesarg
Copy link
Member Author

And just out of curiosity: do you need to call std::move explicitly these days?

That was always the case.

@elwer
Copy link

elwer commented Nov 21, 2023

We have to reflect on the class names in the whitelist in compat.hpp as well. Otherwise, we get errors.
E.g. in compat.hpp:

diff --git i/src/scorepy/compat.hpp w/src/scorepy/compat.hpp
index 378af34..9b772ad 100644 src/scorepy/compat.hpp
--- i/src/scorepy/compat.hpp
+++ w/src/scorepy/compat.hpp
@@ -14,11 +14,13 @@ namespace compat
 {
     /// Region names that are known to have no region enter event and should not report an error
     /// on region exit
-    static const std::array<std::string, 2> exit_region_whitelist = {
+    static const std::array<std::string, 4> exit_region_whitelist = {
 #if PY_MAJOR_VERSION >= 3
-        "threading:_bootstrap_inner", "threading:_bootstrap"
+        "threading.Thread:_bootstrap_inner", "threading.Thread:_bootstrap",
+        "threading.TMonitor:_bootstrap_inner", "threading.TMonitor:_bootstrap"
 #else
-        "threading:__bootstrap_inner", "threading:__bootstrap"
+        "threading.Thread:__bootstrap_inner", "threading.Thread:__bootstrap",
+        "threading.TMonitor:__bootstrap_inner", "threading.TMonitor:__bootstrap"
 #endif
     };
 

TMonitor is a class wrapper for the original Thread class by tqdm. Would be good if that is supported as well.

@bertwesarg
Copy link
Member Author

applied, thanks

@bertwesarg
Copy link
Member Author

@NanoNabla can we just drop Python 2 support please: actions/setup-python#672

Copy link
Collaborator

@NanoNabla NanoNabla left a comment

Choose a reason for hiding this comment

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

Thanks @bertwesarg

@NanoNabla NanoNabla merged commit 6aa9e51 into score-p:master Dec 4, 2023
12 of 13 checks passed
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.

Class name in region name not mentioned when importing module
4 participants