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

#1550 allow passing appconfig to vt during init #1674

Merged
merged 11 commits into from
Mar 2, 2022
Merged

#1550 allow passing appconfig to vt during init #1674

merged 11 commits into from
Mar 2, 2022

Conversation

jstrzebonski
Copy link
Contributor

@jstrzebonski jstrzebonski commented Feb 15, 2022 •
edited
Loading

Fixes #1550

TODO:

  • print options overwritten by CLI arguments
  • clean up ArgConfig struct - if possible move member functions to the anonymous namespace in *.cc file
  • fix unit tests failures (gcc-10, ubuntu, openmpi, no LB)
  • Add documentation
  • Add documentation on appConfig being overwritten by CLI arguments

@jstrzebonski jstrzebonski self-assigned this Feb 15, 2022
@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (clang-5.0, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (clang-3.9, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (gcc-6, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (gcc-5, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

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

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

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

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (nvidia cuda 11.0, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (clang-9, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@codecov
Copy link

codecov bot commented Feb 15, 2022 •
edited
Loading

Codecov Report

Merging #1674 (e151dd3) into develop (4491675) will increase coverage by 0.12%.
The diff coverage is 96.06%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1674      +/-   ##
===========================================
+ Coverage    83.46%   83.58%   +0.12%     
===========================================
  Files          799      799              
  Lines        30934    31186     +252     
===========================================
+ Hits         25819    26068     +249     
- Misses        5115     5118       +3
Impacted Files Coverage Δ
src/vt/configs/arguments/args.h 100.00% <ø> (ø)
src/vt/runtime/runtime.h 100.00% <ø> (ø)
src/vt/collective/startup.cc 56.52% <33.33%> (-19.95%) ⬇️
src/vt/configs/arguments/args.cc 96.05% <94.09%> (+0.05%) ⬆️
examples/hello_world/objgroup.cc 100.00% <100.00%> (ø)
src/vt/collective/collective_ops.cc 88.52% <100.00%> (+28.90%) ⬆️
src/vt/runtime/runtime.cc 76.15% <100.00%> (ø)
tests/unit/runtime/test_initialization.cc 100.00% <100.00%> (ø)
src/vt/runtime/runtime_banner.cc 63.71% <0.00%> (+0.61%) ⬆️

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

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

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (nvidia cuda 10.1, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

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

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (clang-10, alpine, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@github-actions
Copy link

github-actions bot commented Feb 15, 2022 •
edited
Loading

PR tests (clang-10, ubuntu, mpich)

Build for e151dd3

Compilation - successful

Testing - passed

Build log

@jstrzebonski jstrzebonski force-pushed the 1550-allow-passing-appconfig-to-vt-during-init branch from 04d14f7 to 72efeef Compare February 16, 2022 20:53
@jstrzebonski
Copy link
Contributor Author

Printing options predefined in source code, that are overwritten by CLI arguments:

image

image

@jstrzebonski jstrzebonski marked this pull request as ready for review February 17, 2022 13:32
@cz4rs
Copy link
Contributor

cz4rs commented Feb 17, 2022

@jstrzebonski This functionality should be mentioned in documentation and the precedence should be described explicitly (test_initialization.cc helps too 👍).

cz4rs
cz4rs requested changes Feb 22, 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.

Please add a short description into docs (basically a more user-friendly way to tell users what you wrote in unit tests - // CLI args should overwrite hardcoded appConfig).

@jstrzebonski jstrzebonski force-pushed the 1550-allow-passing-appconfig-to-vt-during-init branch from 9ba6ff9 to 82e7e31 Compare February 22, 2022 18:03
@jstrzebonski
Copy link
Contributor Author

jstrzebonski commented Feb 22, 2022 •
edited
Loading

@jstrzebonski This functionality should be mentioned in documentation and the precedence should be described explicitly (test_initialization.cc helps too +1).

@cz4rs Done.

@lifflander lifflander mentioned this pull request Feb 22, 2022
Open
@jstrzebonski
Copy link
Contributor Author

CLI11 parses configuration file before command-line arguments. I added appropriate documentation and two new unit tests covering this behavior.

@jstrzebonski jstrzebonski requested a review from cz4rs February 23, 2022 17:54
cz4rs
cz4rs approved these changes Feb 23, 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.

👍

lifflander
lifflander approved these changes Feb 24, 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 to me!

@nlslatt nlslatt force-pushed the 1550-allow-passing-appconfig-to-vt-during-init branch from b208c0a to 11c1230 Compare March 1, 2022 20:54
@nlslatt
Copy link
Collaborator

nlslatt commented Mar 1, 2022

Note that the automatic rebase feature doesn't retain signatures. I'll fix it.

@jstrzebonski jstrzebonski force-pushed the 1550-allow-passing-appconfig-to-vt-during-init branch from 11c1230 to e151dd3 Compare March 1, 2022 21:08
@jstrzebonski
Copy link
Contributor Author

Note that the automatic rebase feature doesn't retain signatures. I'll fix it.

No problem, I've just force-pushed.

@nlslatt nlslatt merged commit a8be568 into develop Mar 2, 2022
cz4rs pushed a commit that referenced this pull request Mar 4, 2022
@nlslatt @cz4rs
Merge pull request #1674 from DARMA-tasking/1550-allow-passing-appcon…
3e0f559 
…fig-to-vt-during-init

#1550 allow passing appconfig to vt during init
cz4rs pushed a commit that referenced this pull request Mar 4, 2022
@nlslatt @cz4rs
Merge pull request #1674 from DARMA-tasking/1550-allow-passing-appcon…
9807d16 
…fig-to-vt-during-init

#1550 allow passing appconfig to vt during init
cz4rs pushed a commit that referenced this pull request Mar 4, 2022
@nlslatt @cz4rs
Merge pull request #1674 from DARMA-tasking/1550-allow-passing-appcon…
a2eb852 
…fig-to-vt-during-init

#1550 allow passing appconfig to vt during init
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cz4rs cz4rs cz4rs approved these changes

@lifflander lifflander lifflander approved these changes

@JacobDomagala JacobDomagala Awaiting requested review from JacobDomagala

@nlslatt nlslatt Awaiting requested review from nlslatt

@nmm0 nmm0 Awaiting requested review from nmm0

@PhilMiller PhilMiller Awaiting requested review from PhilMiller

Assignees

@jstrzebonski jstrzebonski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow users to pass AppConfig to configuration VT during inititialization
4 participants
@jstrzebonski @cz4rs @nlslatt @lifflander