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.

Sign up for GitHub

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

Jump to bottom

#1636 add API for temporarily enabling/disabling debug prints #1658

Merged
merged 7 commits into from
Feb 1, 2022
Merged

#1636 add API for temporarily enabling/disabling debug prints #1658

merged 7 commits into from
Feb 1, 2022

Conversation

jstrzebonski
Copy link
Contributor

Fixes #1636

@jstrzebonski jstrzebonski self-assigned this Jan 14, 2022
@jstrzebonski jstrzebonski force-pushed the 1636-api-temporarily-enabling-disabling-debug-prints branch from 1e76f5f to db22225 Compare January 14, 2022 11:50
@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (clang-9, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (gcc-5, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (gcc-10, ubuntu, openmpi, no LB)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (clang-5.0, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (clang-3.9, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (gcc-6, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (nvidia cuda 10.1, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (clang-10, alpine, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (nvidia cuda 11.0, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (clang-10, ubuntu, mpich)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (gcc-9, ubuntu, mpich, zoltan)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (gcc-8, ubuntu, mpich, address sanitizer)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@codecov
Copy link

codecov bot commented Jan 14, 2022 •
edited
Loading

Codecov Report

Merging #1658 (c7379fd) into develop (176190c) will decrease coverage by 0.00%.
The diff coverage is 62.19%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1658      +/-   ##
===========================================
- Coverage    83.53%   83.52%   -0.01%     
===========================================
  Files          797      799       +2     
  Lines        30816    30896      +80     
===========================================
+ Hits         25741    25807      +66     
- Misses        5075     5089      +14
Impacted Files Coverage Δ
src/vt/configs/debug/debug_print.h 100.00% <ø> (ø)
src/vt/configs/debug/debug_print.cc 11.42% <11.42%> (ø)
src/vt/runtime/runtime_get.cc 100.00% <100.00%> (ø)
...t/runtime/test_temp_enabling_debug_prints.nompi.cc 100.00% <100.00%> (ø)
src/vt/rdma/group/rdma_group.cc 0.00% <0.00%> (-10.00%) ⬇️
src/vt/context/context_attorney.cc 33.33% <0.00%> (-6.67%) ⬇️
src/vt/messaging/request_holder.h 77.14% <0.00%> (-5.72%) ⬇️
src/vt/runtime/component/diagnostic_enum_format.cc 0.00% <0.00%> (-2.44%) ⬇️
tests/unit/runtime/test_preconfig.nompi.cc 77.77% <0.00%> (-2.23%) ⬇️
src/vt/sequence/seq_context.cc 39.28% <0.00%> (-2.10%) ⬇️
... and 37 more

@github-actions
Copy link

github-actions bot commented Jan 14, 2022 •
edited
Loading

PR tests (gcc-7, ubuntu, mpich, trace runtime, LB)

Build for c7379fd

Compilation - successful

Testing - passed

Build log

@jstrzebonski jstrzebonski requested a review from nmm0 January 14, 2022 15:44
@jstrzebonski
Copy link
Contributor Author

jstrzebonski commented Jan 14, 2022 •
edited
Loading

@nmm0 Is it something that might come in handy for your use cases (take a look at simplified usage in unit tests)?

@nmm0
Copy link
Collaborator

nmm0 commented Jan 17, 2022

@nmm0 Is it something that might come in handy for your use cases (take a look at simplified usage in unit tests)?

Yes, this is perfect, especially the scoped version. In my use cases I often want to get the prints from a specific function.

nmm0
nmm0 approved these changes Jan 17, 2022
View reviewed changes
Copy link
Collaborator

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Only thing I'd want to add is some documentation just to make developers aware of the feature.

@jstrzebonski
Copy link
Contributor Author

Looks good to me! Only thing I'd want to add is some documentation just to make developers aware of the feature.

Documentation added.

@jstrzebonski jstrzebonski marked this pull request as ready for review January 18, 2022 13:51
cz4rs
cz4rs approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@cz4rs cz4rs left a comment

Choose a reason for hiding this comment

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

Looks good to me!

nmm0
nmm0 approved these changes Jan 20, 2022
View reviewed changes
Copy link
Collaborator

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

+1

lifflander
lifflander requested changes Jan 20, 2022
View reviewed changes
Copy link
Collaborator

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

This looks great to me. The only change I would suggest is checking to see if VT is live, and then printing an appropriate error/exception/abort if it isn't.

@jstrzebonski
Copy link
Contributor Author

This looks great to me. The only change I would suggest is checking to see if VT is live, and then printing an appropriate error/exception/abort if it isn't.

Done

jstrzebonski
jstrzebonski commented Jan 24, 2022
View reviewed changes
src/vt/configs/debug/debug_print.cc Outdated
} \
\
ScopedModifier_##opt##_##val createScopedModifier_##opt##_##val() { \
if (not curRT) { \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to tell if vt's been initialized already?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could just check if (vt::theConfig()) which might make a little more sense in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, checking that looks better in this context, but internally it uses two pointers before it returns config, so if vt is not initialized, I think the program would (at best) segfault.

vt::arguments::AppConfig *theConfig() { return &CUR_RT->theArgConfig->config_; }

lifflander
lifflander requested changes Jan 25, 2022
View reviewed changes
src/vt/configs/debug/debug_print.h Outdated
vtAbort("Trying to read config when VT is not initialized"); \
} \
\
theConfig()->vt_debug_##opt = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to qualify all of these calls to theConfig() with ::vt::theConfig() because they might not be in the VT namespace for application code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jstrzebonski jstrzebonski force-pushed the 1636-api-temporarily-enabling-disabling-debug-prints branch from 88ea473 to 95a304b Compare January 25, 2022 10:11
@jstrzebonski jstrzebonski force-pushed the 1636-api-temporarily-enabling-disabling-debug-prints branch from 95a304b to 75f6630 Compare January 25, 2022 10:38
@jstrzebonski
Copy link
Contributor Author

There are still some funky compilation errors with fcontext and unity build enabled.

@lifflander lifflander self-requested a review January 25, 2022 18:11
lifflander
lifflander approved these changes Jan 25, 2022
View reviewed changes
Copy link
Collaborator

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

Looks good once we can get CI passing!

@lifflander lifflander mentioned this pull request Jan 25, 2022
Open
@lifflander lifflander mentioned this pull request Feb 1, 2022
Closed
@lifflander lifflander merged commit b1e3867 into develop Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmm0 nmm0 nmm0 approved these changes

@cz4rs cz4rs cz4rs approved these changes

@lifflander lifflander lifflander approved these changes

@PhilMiller PhilMiller Awaiting requested review from PhilMiller

@JacobDomagala JacobDomagala Awaiting requested review from JacobDomagala

Assignees

@jstrzebonski jstrzebonski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add API to temporarily enable/disable debug prints
5 participants
@jstrzebonski @nmm0 @lifflander @cz4rs @JacobDomagala