Extension: Resource manager design

[yhmtsai]

This PR adds the extension resource manager. The purpose is to add a way to search/build/insert data from json (possible yaml or command-line support in future). Use json as the initial choice because json does not need indent consideration and contain native nest structure. Thus, it can be generated in code not just from file. The design is trying to keep the same style of ginkgo usage and the logic such as

• the parameters in constructor of matrix default may comes from other input not always a single default value.
• the factory default is from ginkgo self
• also allow nest construction or step-by-step construction
• trying to make the code close to copy-paste.

some function/class short description:

• Generic<T, U>::build is the actual implementation.
• GenericHelper is to use the correct Generic<T, U> U is T by default, but U is the base_type from T if T is derived from Factory but not LinOpFactory or CriterionFactory itself
• ResourceManager also contains build_item, which will call Generic build function but adds the generated object if it contains name
• create_from_config<enum_type, enum_item> is the function inside selection. the implementation of it should contains the further template type selection if needs
• span<tt_list<K1, K2>, tt_list<T1, T2>, tt_list<U1, U2>>::type = tt_list<type_list<K1, T1, U1>, type_list<K1, T1, U2>, ...>
• thanks to integral_constant, we can put the template value as type and transfer to the correct template such that we can still use selection on a tt_list (type_list).

This PR may change a lot during discussion/review. This PR is somehow between WIP (for complete tests) and ready-for-review for the design. I think it is okay for a start point for discussing.

possible todo:

• [ ] thread-safe on map operation
• [ ] add Type test to test all possible type
• [ ] change tt_list and possible the span implementation

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
135 Code Smells

42.6% Coverage
5.5% Duplication

[codecov[bot]]

Codecov Report

Attention: 1602 lines in your changes are missing coverage. Please review.

Comparison is base (4c3320c) 90.61% compared to head (aeb7586) 88.58%. Report is 1874 commits behind head on develop.

:exclamation: Current head aeb7586 differs from pull request most recent head b98e766. Consider uploading reports for the commit b98e766 to get more accurate results

Files	Patch %	Lines
...include/resource_manager/base/rapidjson_helper.hpp	29.87%	162 Missing :warning:
...manager/include/resource_manager/matrix/hybrid.hpp	0.00%	68 Missing :warning:
...er/include/resource_manager/preconditioner/ilu.hpp	0.00%	43 Missing :warning:
...include/resource_manager/factorization/par_ict.hpp	0.00%	41 Missing :warning:
...nclude/resource_manager/factorization/par_ilut.hpp	0.00%	41 Missing :warning:
...mple-solver-separate/rm-simple-solver-separate.cpp	0.00%	40 Missing :warning:
...include/resource_manager/preconditioner/jacobi.hpp	0.00%	39 Missing :warning:
.../include/resource_manager/factorization/par_ic.hpp	0.00%	38 Missing :warning:
...include/resource_manager/factorization/par_ilu.hpp	0.00%	38 Missing :warning:
...er/include/resource_manager/multigrid/amgx_pgm.hpp	0.00%	38 Missing :warning:
... and 45 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #800      +/-   ##
===========================================
- Coverage    90.61%   88.58%   -2.04%     
===========================================
  Files          508      563      +55     
  Lines        44334    44550     +216     
===========================================
- Hits         40173    39464     -709     
- Misses        4161     5086     +925     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[MarcelKoch]

One point that could be interesting to discuss, is the adding of data into the resource manager. Currently, a rapidjson::Value object has to be passed to the read function of the RM. I would suggest adding the possibility to fill the database by passing just a filename to the corresponding json file, i.e.

Rm resource_manager;
resource_manager.read("data.json")

I think that would be nice to have from a usability standpoint. Also, perhaps a corresponding constructor could be added.

[yhmtsai]

@MarcelKoch Yes, it will be added

[upsj]

This is really impressive work. I think there are probably better ways to make the interactions with factories more smooth, but in general, this sounds really useful.

Before I continue here, let me reiterate what I believe to be the initial aim of this whole effort: We want to provide a way to initialize solvers for a system matrix generically based on a configuration file. This involves simple things like stopping criteria and Krylov solver parameters, but also really complicated setups like mixed precision solves or multigrid preconditioners, which can have a large number of (possibly nested) parameters.

The main thing which I would like to consider though is the scope of this setup. Right now, the scope looks huge to me. Executors, matrix LinOps, file IO, LinOp factories, you are basically able to represent everything in Ginkgo. And honestly, I don't believe this is a good thing. It looks to me like this kind of global object registry could promote really bad code (storing everything in the registry instead of variables, bad type safety since we do everything dynamically) and become another global object next to Executor users need to carry around everywhere in their code. I would probably be fine with storing configurations inside a global registry though, since that seems to be what most applications need to do anyways for their parameters, so we could build on top of that.

I believe the current JSON setup does not do a good job yet showing the nested nature of what it is trying to represent. Instead of having individual (named) objects listed, like they would be stored in the resource manager, they could be represented in their hierarchy:

{
    "type":"solver::Cg",
    "stopping_criteria": {
        "iterations": 100,
        "time": 0.5,
        "rel_residual": 1e-6
    },
    "preconditioner": {
        "type": "multigrid::Multigrid",
        "coarsening": {
            "type": "coarsening::AMGx",
            "agg": 2
        },
        "pre_smoother": {
            "type": "preconditioner::Jacobi",
            "block_size": 1
        },
        "post_smoother": {
            "type": "preconditioner::Jacobi",
            "block_size": 1
        },
        "coarse_solver": {
            "type": "solver::gmres",
            ...
        }
    }
}

That would also mean we no longer need to give names to objects, and thus could eliminate the need to store objects inside a resource manager.

If we do go for a resource manager, there are some things which I would like to keep out of it for now, in decreasing order of importance

• I believe resource managers should never create executors. At best, they should be using a pre-existing executors or be used to provide parameters for a generic executor factors, but that executor should be a variable outside of the resource manager, not a named object inside the resource manager's storage.
• I am not convinced resource managers should do matrix IO. We have a pretty simple API for that, and reading a matrix from file is probably not a common use case for applications.
• I believe it would be best if we kept executor settings out of the resource manager design, using the executor belonging to the resource manager to initialize all objects we are looking at.
• I am not yet sure how best to represent mixed precision setups, maybe a type parameter for the factories?

Finally, it might be helpful for users to have something like a Python script that converts such a JSON file into a piece of code that does exactly the initialization the configuration describes.

Also, completely independent of that, it might be useful in the future if users were able to specify a matrix type to be used to store the system matrix in the solver, converting the input matrix in the generation step if necessary. Maybe by initializing an empty matrix of that type in the solver factory parameters?

[yhmtsai]

I think these type needs the dynamic? In short, these are optional in usage.

the example you show, it can be done with the standalone free function or use resource_manager. In resource_manager, it will depends on how to use it. If use build_item<LinOp>, you will get LinOp and do not need the name. If use build_itemsolver::Cg, you will get Cg pointer directly without any dynamic cast. also the name is optional. if use read, because it does not contain type information such that decide the return type, it must to contain the name to store data and get them from the database. Here will require dynamic_cast if need the Cg pointer. The name is only required when you build without any type information in code. the nest object does not require name anymore. but if you put the name inside the object, the resource manager will putting them into database, and standalone function always ignore the name. you example already works. (except for the stop criterion)

Also, the executor can be given when you build_item from ResourceManager. It does not require the executor built and stored inside ResourceManager. Standalone free option supports this for sure. They all can use pre-executor

The MatrixIO does not need to be called. you can create the matrix from config and do anything you want on this matrix. And also, it should have something simple operation such as fill.

the third one already works.

all parameters or template will depends on the ginkgo design.

I thought the solver generation already take care the type of the input matrix. (convert to valid matrix or throw error)

[MarcelKoch]

I would like to add, that I'm (somewhat strongly) in favor of keeping the name of the configuration. The version mentioned by @upsj without storing a name allows only for exactly one solver configuration. Perhaps you could differentiate by solver type, but two solvers of the same type with different configurations would not be possible.

I think users should have the possibility to use multiple solvers, one example for this I could think of are multiphysics simulation. In these cases, it makes sense to use different solvers for the different subsystems.

[upsj]

I think you may have misunderstood my example a bit. This is only meant to be a single configuration for a specific LinOpFactory. An application of course needs to be able to use more than one such configuration. To be a bit more specific: I am only arguing against names for objects (i.e. matrices, executors, solvers, ...). Named configurations on the other hand seem perfectly fine, since they are very lightweight.

For nested objects, it might even make sense to allow specifying strings instead of JSON objects to refer to previously listed configurations. That would also allow people to reuse things like coarse solver configurations for multiple setups. It does sound like your setup already allows for both nesting and referring to configurations by their name, Mike, so we are probably already on the same page about this.

[MarcelKoch]

Ah, I see. So I guess this directly relates to your other point, should we only store the configuration or the created object.

IMO, storing only the configuration and always building the objects when requested results in a cleaner separation of concerns. However, I guess that implementation-wise there would be not that big of a difference. Sidenote: A change in that direction could justify renaming it to config_manager.

Still, I agree with @upsj that storing objects is unnecessary, since the important part is that an object can be created directly from a file input.

[yhmtsai]

a difference from @upsj is I stored the object with name not the config with name. I should be able to add the config part. named object can make us use generated function such as generated_preconditioner.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
144 Code Smells

40.7% Coverage
5.3% Duplication

[ginkgo-bot]

Error: The following files need to be formatted:

extension/resource_manager/examples/rm-simple-solver-separate/rm-simple-solver-separate.cpp
extension/resource_manager/examples/rm-simple-solver/rm-simple-solver.cpp
extension/resource_manager/examples/standalone-simple-solver/standalone-simple-solver.cpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

[sonarqubecloud[bot]]

SonarCloud Quality Gate failed.   

2 Bugs
0 Vulnerabilities
0 Security Hotspots
324 Code Smells

15.1% Coverage
7.3% Duplication

[ginkgo-bot]

Error: The following files need to be formatted:

extension/resource_manager/examples/rm-simple-solver-separate/rm-simple-solver-separate.cpp
extension/resource_manager/examples/rm-simple-solver/rm-simple-solver.cpp
extension/resource_manager/examples/standalone-simple-solver/standalone-simple-solver.cpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo