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

resource: Refactor policy code + other cleanup #325

Merged
merged 6 commits into from May 10, 2018

Conversation

Projects
None yet
4 participants
@dongahn
Copy link
Contributor

dongahn commented Apr 27, 2018

This PR further refactors the resource code beyond Issue #322 with a goal to position the code for future work (e.g., #319). Please don't merge this yet. I will keep refactoring and cleaning up other parts of the resource layer.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 27, 2018

Coverage Status

Coverage decreased (-0.4%) to 74.749% when pulling 87e1b4a on dongahn:resource_refactor into 0d9a9ff on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 27, 2018

Codecov Report

Merging #325 into master will decrease coverage by 0.31%.
The diff coverage is 52.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
- Coverage    73.6%   73.29%   -0.32%     
==========================================
  Files          56       59       +3     
  Lines        9791     9853      +62     
==========================================
+ Hits         7207     7222      +15     
- Misses       2584     2631      +47
Impacted Files Coverage Δ
resource/generators/gen.cpp 83.95% <ø> (ø)
resource/evaluators/edge_eval_api.hpp 84% <ø> (ø)
resource/utilities/command.hpp 100% <ø> (ø) ⬆️
resource/traversers/dfu_impl.hpp 100% <ø> (ø)
resource/traversers/dfu.cpp 68.29% <ø> (ø)
resource/generators/spec.cpp 54.54% <ø> (ø)
resource/evaluators/edge_eval_api.cpp 62.02% <ø> (ø)
resource/schema/color.cpp 71.42% <ø> (ø)
resource/schema/infra_data.cpp 35.18% <ø> (ø)
resource/generators/spec.hpp 100% <ø> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d9a9ff...87e1b4a. Read the comment docs.

@dongahn dongahn force-pushed the dongahn:resource_refactor branch from 5b2a759 to f51b722 Apr 30, 2018

@dongahn dongahn requested review from SteVwonder and tpatki Apr 30, 2018

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Apr 30, 2018

@tpatki and @SteVwonder: This should be ready for your review. Again, these are mostly refactoring for future work and later extendibility though there are some minor code changes.

I expect your advanced scheduling policies to land resource/policies folder, so please let me know what you think. Also, for other developers who may want to extend this infrastructure w/ other types of traverses and etc, I've added a doxygen config file, which generally generates pretty neat pages for C++ code.

@SteVwonder
Copy link
Member

SteVwonder left a comment

The refactor LGTM!

I cannot commit to adding an I/O-aware policy to resource/policies until June, but once I start working on that, I'll be sure to provide additional feedback then.

private:
// resource types that will be used for scheduler driven aggregate updates
// Examples:
// m_pruning_type["containment"]["rack"] -> {"node"}

This comment has been minimized.

@SteVwonder

SteVwonder May 1, 2018

Member

Just to clarify, this automatic aggregation at a given level has not been implemented yet, correct? Only the "any level" aggregation, as in the example below, has been implemented?

If so, should we make calling set_pruning_type with anchor_type != ANY_RESOURCE_TYPE log a warning?

This comment has been minimized.

@SteVwonder

SteVwonder May 1, 2018

Member

@dongahn: sorry about this. I commented directly on an individual commit's code, rather than commenting on the overall view. So GitHub thinks the comment is out-of-date because there commits that occur after, but it does appear that this code exists in the final set of changes. I'll keep this UI quirk in mind for the future.

This comment has been minimized.

@dongahn

dongahn May 1, 2018

Author Contributor

No problem.

Just to clarify, this automatic aggregation at a given level has not been implemented yet, correct? Only the "any level" aggregation, as in the example below, has been implemented?

This is my next step. So a log warning shouldn't be necessary.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented May 1, 2018

@SteVwonder, thanks! IO aware policy implementation isn't a priority. Primarily I wanted to have a review on this PR from someone who will add policies.

dongahn added some commits Apr 27, 2018

resource: Introduce scheduling policy code folder
Plus, refactor the existing policy codes into their own
header/source files.

This folder is expected to grow as various new
advanced scheduling policy codes will be contributed.

This will include @tapasya's node-performance
variability-aware policy and @SteVwonder's
IO bandwidth-aware policy written as a dervied class
of dfu_match_cb_t class.
resource: Refactor the matcher callback layer
Move matcher_data.hpp into matcher.hpp and matcher.cpp

Introduce matcher_util_api_t class to contain utility
methods needed by matcher callback class and other objects.
And move select_count method from matcher_data_t to
matcher_util_api_t and rename it to calc_count.

Adjust other parts of the matcher layer for these changes.
resource: Refactor Scheduler-Driven Aggregate Update (SDAU) helper
Refactor this logic into matcher_util_api_t class.

Introduce three new methods that allows the upper layer
to test 1) the total set of resource type per subsystem
that is subject to SDAU; 2) the set of resource type
per upper resource type per subsystem.

Slightly modify the upper layer to use the new
abstraction.
resource: Move sources of the same topic into their own folders
Refactor to improve extendibility.

Move resource data and graph into "schema" directory

Move DFU traverser code into "traversers" directory, the folder
where different types of graph traverser can be introduced.

Move dfu_match_cb.[h|c]cpp and matcher.[h|c]pp into "policies/base"
directory.

Move scoring_api and other classes used for resource vertex
and edge evaluation into the "evaluators" folder.

Move resource generator code into "generators" directory and
shorten the file names.
resource: Add a doxygen config file
Help others understand the overall architecture of
the resource scheduling infrastructure code.

`cd resource/doxygen`
`doxygen doxy_conf.txt`

This will generate ./html, ./latex and ./man.
You can point a web broswer to ./html/index.html to
start to traverse the doxygen-generated source documents.

Similarly, `cd ./latex` and `make` will produce
a pdf file (refman.pdf) which you can view with
a pdf viewer.

Currently, this isn't hooked into our build
system and also isn't distributed.

@dongahn dongahn force-pushed the dongahn:resource_refactor branch from 8334b8c to 87e1b4a May 8, 2018

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented May 8, 2018

Ok. Rebased. This can go in as is as @SteVwonder already approved it. @tpatki and I can revisit the changes later if needed.

@SteVwonder SteVwonder merged commit 6aa70d5 into flux-framework:master May 10, 2018

2 of 4 checks passed

codecov/patch 52.94% of diff hit (target 73.6%)
Details
codecov/project 73.29% (-0.32%) compared to 0d9a9ff
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.4%) to 74.749%
Details
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented May 10, 2018

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.