DTL merge conflct resolution

flux-framework / dyad

DYAD: DYnamic and Asynchronous Data Streamliner

GNU Lesser General Public License v3.0

6 stars 5 forks source link

DTL merge conflct resolution #73

Closed JaeseungYeom closed 5 months ago

JaeseungYeom commented 6 months ago

PR #58 has been diverged from the main branch due to recent CMake-based refactoring as well as file locking and reinitialization features.

This PR is to resolve the merge conflicts.

JaeseungYeom commented 6 months ago

Will need to find a way not to explicitly pass the perf handle. Profiling should avoid adding extra overhead as much as possible however small that will be.

JaeseungYeom commented 6 months ago

I had to change perf_handle to self->perf_handle in line 812 of src/dyad/dtl/ucx_dtl.c to build.

hariharan-devarajan commented 5 months ago

@JaeseungYeom so currently DYAD ctx is user facing as well as internal stuff. We should segregate that and pass it on all functions. This will keep our internal apis very flexible.

hariharan-devarajan commented 5 months ago

The way we do signature for logging and profiling it is not very easy to extend. I feel we should have an internal ctx to functions and pass it to all functions. Including logging and profiling

JaeseungYeom commented 5 months ago

@ilumsden Please check if everything is in order. I intentionally left out Caliper enabling cmake flag and CI test to reduce conflict. However, you can still add it locally and test. The CI currently is not working, which I have been seeing recently. That could be another issue to track down.

JaeseungYeom commented 5 months ago

The way we do signature for logging and profiling it is not very easy to extend. I feel we should have an internal ctx to functions and pass it to all functions. Including logging and profiling

We can discuss this All those user facing options are directly used by the runtime. But there will be things that are used by the runtime while not being visible to users.

JaeseungYeom commented 5 months ago

@hariharan-devarajan is working on fixing the bug in module's argparse.

The header is indirectly included from perf. Perf should not be included when profiling is off.
"flux/optparse.h" does not seem portable. I think we either embed the CRAN code as base64 or use a standard library. e.g., C getopt (or boost program_options in case of c++)

JaeseungYeom commented 5 months ago

We merged PR #66 which is based the squashed version of this PR but lacking Caliper instrumentation. We will integrate Caliper in a separate PR.