Adds a Data Transport Layer to DYAD to support different ways of transferring data #24

ilumsden · 2023-02-28T17:50:10Z

This PR adds a new Data Transport Layer (DTL) to the DYAD module and DYAD Core library to allow us to support different ways to transfer data (i.e., transfer backends) between producer and consumer. Through this DTL, this PR also adds a new UCX-based transfer backend.

To set the backends, users will set the new DYAD_DTL_MODE environment variable. This variable must be set to the same value for both the clients (i.e., user applications that use the C or C++ APIs) and the DYAD module. Valid values are:

FLUX_RPC (~~default~~): uses Flux's RPC framework to transfer data. This is the same as before
UCX (default): uses UCX to transfer data

The code for the DTL can be found in src/core/dtl (for DYAD Core's side) and src/modules/dtl (for the DYAD module).

This PR also adds a couple of QOL improvements. Most notably, it adds a new dyad_init_env function to the Core library. This function will initialize the DYAD context (dyad_ctx_t) using the environment variables defined in src/core/dyad_env.h. By providing this function in Core, we can define a baseline environment variable-based initialization for all APIs.

ilumsden · 2023-05-18T20:54:51Z

@JaeseungYeom I've removed the last bit of excess logging and retested. So, this PR is now ready-for-review.

ilumsden · 2023-06-20T23:04:09Z

Thanks to some discussion with @grondo, we identified that a lot of symbols are being exported from DYAD's libraries that shouldn't be exported. I am in the process of correcting this.

One question related to symbol exporting @JaeseungYeom: what do we want to do about dyad_sync_directory? That function is conditionally compiled, and, as far as I know, there's no way to tell libtool to conditionally export a symbol.

JaeseungYeom · 2023-07-06T22:04:27Z

I think you can treat dyad_sync_directory similarly to other DAYD internal functions.

ilumsden · 2023-07-12T01:07:18Z

Notes from review:

Refactor DTL interfaces to use struct + factory method
Merge two DTL libraries into one
Send ACK RPC response to validate that RPC correctly reached UCX send
Add timeout to wait for UCX send/recv
Add delay of 10 us (i.e., usleep) if ucp_worker_progress takes less than 10 us
Check how timeouts work in UCX. If I can't find anything, at least add a TODO
Cache endpoints w/ LRU replacement (new PR for this)
Move tag creation to broker, and make tag (broker_rank << 32) | counter (counter is unique ID in broker)
Add parens around UCX_CHECK and arg to UCX_CHECK

JaeseungYeom · 2023-07-12T18:29:12Z

Send ACK RPC response to validate that RPC correctly reached UCX send
Add timeout to wait for UCX send/recv
Check how timeouts work in UCX. If I can't find anything, at least add a TODO

If 3 is available and 2 is working, do we still need 1?

hariharan-devarajan · 2023-07-12T18:57:03Z

Send ACK RPC response to validate that RPC correctly reached UCX send

Add timeout to wait for UCX send/recv

Check how timeouts work in UCX. If I can't find anything, at least add a TODO

If 3 is available and 2 is working, do we still need 1?

I believe they give different errors:

identifies whether we were able to setup ucx endpoint for now doing data transfer.
and 3. to me are ways to know if ucx transfer failed. I think if ucx has timeouts we should use that (that is 3) if not we should build some simple version of that (that is 2)

JaeseungYeom · 2023-07-13T16:42:34Z

I agree with that trying to see if option 3 is available first and then do option 2. For 1, it sounds to me it does not add any other functionality if either 2 or 3 works. If endpoint setup fails, would the entire DYAD shutdown or do we expect to retry? If the latter, I see it could be useful.

hariharan-devarajan · 2023-07-13T16:50:20Z

The only benefit is detection. Hanging errors are hard to detect. And this would lead to one such case.

JaeseungYeom

You should add ```#if defined(__cplusplus)
extern "C" {
#endif // defined(__cplusplus)

#if defined(__cplusplus)
};
#endif // defined(__cplusplus)``` to headers. I am still going through this PR, but you can start addressing.

configure.ac

JaeseungYeom · 2023-07-28T17:35:39Z

src/core/dyad_core.c

    const dyad_ctx_t* ctx,
    const dyad_kvs_response_t* restrict kvs_data,
    const char** file_data,
-    int* file_len,
-    flux_future_t** f)
+    size_t* file_len)


add restrict. Not just here but any non const pointer in any other function prototypes.
Also, there is restrict used somewhere else. I remember you did some checking in configure where restrict is available or restrict is available. You need to unify these to the one detected as available.

add them to where ever appropiate
https://stackoverflow.com/questions/13307190/restrict-keyword-and-pointers-inside-structs
https://stackoverflow.com/questions/66646856/pointer-to-pointer-aliasing-and-the-restrict-keyword

This can be a separate PR if that works better

I added restrict to the latest commits. However, I should mention that, due to stuff with the Flux handle, I can't add it in many places without telling the compiler incorrect things (which will break the build). I think there were only 3 places where I could definitely add it.

You can show me the problem later.

JaeseungYeom · 2023-07-28T18:13:52Z

src/core/dyad_core.c

+                fprintf (stderr, "Invalid DTL mode provided through %s. \
+                        Defaulting to UCX\n", DYAD_DTL_MODE_ENV);
+            }
+            dtl_mode = DYAD_DTL_UCX;


I remember we had this discussion, and somehow agreed to use UCX as the default. However, now that I think about it, it might be better to use flux rpc because it will be available as long as dyad depends on flux. I know this will change in the future but I am not sure if UCX is universally available. If you think that is the case, this is fine. Otherwise, it should be flux rpc. Or we somehow detect the UCX dependency has been picked up during configuration, a MACRO variable can be set which helps to determine the appropriate default. If needed, this could be another PR.

I think having UCX as default makes sense for the time being. Most systems nowadays have UCX, mainly because its used by MPI. Also, right now, the UCX dependency is required, so DYAD won't build without UCX.

We can think more about this later, but I think that should be a separate PR if we change it.

libfabric is another to consider. AWS EFA is based on libfabric for example. So, we should definitely detect what is available and use it as the default instead of assuming UCX availability.

This is can be a followup PR. Even if libfabric is not supported, UCX availability should be checked.

Requiring UCX as the dependency is also problematic in the short term. Vanessa would not be able to try this if that is the case.

JaeseungYeom · 2023-07-28T18:20:31Z

src/core/dyad_core.c

+    flux_future_t *f;
+    json_t* rpc_payload;
+    DYAD_LOG_INFO (ctx, "Packing payload for RPC to DYAD module");
+    rc = ctx->dtl_handle->rpc_pack (


This pattern is somewhat akward.
I will have to look into rpc_pack() to see why it has to work this way.
However, if ctx->dtl_handle->rpc_pack() always expects ctx->dtl_handle to operate correctly, there can be a better interface.
Edit: I see why you have to do that. We can merge it as is and try find a better interface later. Least we can do is using some macro or a global variable.

I agree its awkward. I spent quite a bit of time looking for an alternative, but I couldn't find one.

A macro could work for simplifying this, but I'd highly recommend against a global variable. We don't necessarily want the context to always be shared across functions.

I do not like global variable approach as well. However, context does not need to be shared for the sake of that. Only dtl handle need to be global, which will be read-only once initialized.

src/core/dyad_core.h

src/dtl/dyad_dtl_impl.c

JaeseungYeom · 2023-07-28T23:11:22Z

src/dtl/flux_dtl.c

+        "upath",
+        upath
+    );
+    if (errcode < 0) {


TODO: Check this with a macro (DYAD_IS_ERROR) as it is done some other places. Also, rc is better than errorcode.

There are similar lines.

This check should not use DYAD_IS_ERROR because it's not checking a DYAD return code. It's checking a Flux return code. The choice of variable name was also intentional to emphasize this difference.

It does not need to be DYAD_IS_ERROR. It can be FLUX_IS_ERROR for example. error code is misleading because it is not necessarily a code for error like errno.

Just pushed a new commit that deals with this.

src/dtl/dyad_dtl.h

JaeseungYeom · 2023-07-28T23:18:47Z

src/modules/dyad.c


-    if (flux_msg_get_userid (msg, &userid) < 0)
-        goto error;
+    if (!flux_msg_is_streaming (msg)) {


We should have a discussion on what the best terms is for "streaming" later.

I agree. FWIW, in this context, the term "streaming" refers to Flux's streaming RPCs (i.e., an RPC which can send multiple return messages and is ended with an error return message with errno set to ENODATA).

src/dtl/dyad_dtl_impl.c

src/core/dyad_core.c

JaeseungYeom · 2023-08-01T13:41:09Z

src/dtl/dyad_flux_log.h

@@ -17,11 +21,14 @@
    do {                            \
    } while (0)
 #else
-#define DYAD_LOG_INFO(dyad_ctx, ...) \
-    flux_log (dyad_ctx->h, LOG_INFO, __VA_ARGS__)
+#define DYAD_LOG_INFO(dyad_ctx, ...) flux_log (dyad_ctx->h, LOG_INFO, __VA_ARGS__)


Since there are not spaces after function-like macros in flux-core, I'd argue that we shouldn't add spaces in DYAD either.

JaeseungYeom · 2023-08-01T13:41:21Z

src/dtl/dyad_rc.h

+
+typedef enum dyad_core_return_codes dyad_rc_t;
+
+#define DYAD_IS_ERROR(code) ((code) < 0)


src/dtl/flux_dtl.c

JaeseungYeom · 2023-08-01T13:47:27Z

src/dtl/dyad_rc.h

+
+#define DYAD_IS_ERROR(code) ((code) < 0)
+
+#define FLUX_IS_ERROR(code) ((code) < 0)


src/utils/base64/base64.c

These sublibraries provide a common interface to sending/receiving data using different tools. The tools currently supported in the DTL sublibraries are: * Flux RPC (i.e., how DYAD has previously moved data) * UCX To control which tool is used, users can set the DYAD_DTL_MODE environment variable for the APIs. For the module/service, users specify which tool to use by passing a second argument on the command line. For both APIs and the module/service, the default DTL mode is Flux RPC.

…oad again

…codes for DYAD based on DTL

hariharan-devarajan

Here are some of my feedback in the code.

hariharan-devarajan · 2023-07-11T22:23:07Z

src/core/dyad_core.c

+        goto get_done;
+    }
+    DYAD_LOG_INFO (ctx, "Receive RPC response from DYAD module");
+    rc = dyad_dtl_recv_rpc_response(ctx->dtl_handle, f);


We should do a ftell on the file and get the size and then we can do the response back.

I agree. However, we already discussed doing this as part of a separate PR.

We should do this alongside #30

I agree with both of you.

hariharan-devarajan · 2023-09-06T21:27:13Z

.clang-format

To me this feels like just formatting change which switches the order. I would recommend to revert this as its unrelated to the actual change.

There actually are a handful of actual changes to .clang-format. Those changes resolve several complications/annoyances, particularly regarding the PerfFlow Aspect annotations messing up return type formatting.

I think Ian's approach is reasonable to avoid manual formatting and productivity decrease although I do not like the solution for the long term. We want the annotation line separate from the function signature, and we want to avoid manual formatting. So far, there is no solution known to us. So, this is an alternative.

hariharan-devarajan · 2023-09-06T21:34:45Z

src/core/Makefile.am

+	$(FLUX_CORE_LIBS)
+libdyad_core_la_CFLAGS = \
+	$(AM_CFLAGS) \
+	-I$(top_srcdir)/src/utils \


Adding -I for finding header files within such cases would make it harder to install the correct files later. The convension is to use till src only in -I so that other libraries can correctly link to DYAD at runtime.

Another general project level comment is that it is good to make distinction between public header files and private header files within a project.

This will help the installer only install the header files expected to be used by applications and rest would not be installed in include but would be compiled together in the so itself.

I absolutely agree with you @hariharan-devarajan, but I did this because we wanted to stay (relatively) consistent with Flux. This flux-core repo does something similar to this in its Makefile.ams.

However, regardless of what Flux does and how much we want to follow that, I also believe that any changes we might want to make regarding organization of files should belong in a separate PR.

I agree that we should put more effort to clean up installation and make distinction between public headers and private headers. This can be worked on in a followup PR.

hariharan-devarajan · 2023-09-06T21:36:04Z

src/core/dyad_core.c

 #include <libgen.h>
 #include <unistd.h>

-#include "dyad_core.h"
-#include "dyad_flux_log.h"
+#include "dyad_dtl_impl.h"


The our internal header would change to refer from src

#include dtl/dyad_dtl_impl.h
#include utils/murmur3.h

and so on.

I think I better understand what you're saying about the headers now that I read this comment, but, again, I believe such a change should be its own PR.

I agree with both of you. It needs to be changed and should be done in a separate PR.

src/core/dyad_core.c

src/wrapper/wrapper.c

hariharan-devarajan · 2023-09-06T22:05:33Z

src/wrapper/wrapper.c

@@ -199,8 +142,7 @@ int open (const char *path, int oflag, ...)
    }

    if (!(ctx && ctx->h) || (ctx && !ctx->reenter)) {
-        IPRINTF (ctx, "DYAD_SYNC: open sync not applicable for \"%s\".\n",
-                 path);
+        IPRINTF (ctx, "DYAD_SYNC: open sync not applicable for \"%s\".\n", path);


We should use the same Logging mechanism DYAD_LOG_INFO

src/wrapper/wrapper.c

hariharan-devarajan · 2023-09-06T22:06:03Z

src/wrapper/wrapper.c

@@ -238,7 +180,8 @@ FILE *fopen (const char *path, const char *mode)
    }

    if (!(ctx && ctx->h) || (ctx && !ctx->reenter) || !path) {
-        IPRINTF (ctx, "DYAD_SYNC: fopen sync not applicable for \"%s\".\n",
+        IPRINTF (ctx,


DYAD_LOG_INFO

We should explore unifying the logging mechanisms in DYAD. I'll create an issue for that so we can track it as future work.

I created #44 to track this.

The current logging is carry-over from my old code, and it needs to be refreshed and cleaned up. Some of the things got partially removed over refactoring processes. We can discuss a better structure and approaches for future-proofness, succinctness, and clarity.

hariharan-devarajan · 2023-09-06T22:09:09Z

src/wrapper/wrapper.c

-    }
+    // if (ctx == NULL) {
+    //     dyad_wrapper_init ();
+    // }

    func_ptr = (fopen_ptr_t)dlsym (RTLD_NEXT, "fopen");


All this logic of moving to next symbol can be abstracted using macro for each case.

#define MAP_OR_FAIL(func_) \ if (!(real_##func_##_)) { \ real_##func_##_ = (real_t_##func_##_)dlsym(RTLD_NEXT, #func_); \ if (!(real_##func_##_)) { \ fprintf(stderr, "failed to map symbol\n"); \ } \ } #else #define MAP_OR_FAIL(func) #endif

I recommend using GOTCHA tool which handles this more dynamically.

I agree that switching the GOTCHA in the future is the right move. That definitely doesn't belong in this PR though. If we don't have one yet, I'll create an issue for switching from "raw" LD_PRELOAD to GOTCHA.

Issue #43 has been created to track adding GOTCHA support

It make sense to address it in a separate PR. @hariharan-devarajan please find time with us to explain the benefits from the users perspective as well as developer's.

* Abstracts DYAD module callback name to a const global variable in dyad_dtl_impl.h * Removes an unneeded comment from the Makefile.am in wrapper * Removes commented-out code that invokes dyad_wrapper_init from wrapper.c * Updates header guards to be consistant

…it is a constant

ilumsden added the enhancement New feature or request label Feb 28, 2023

ilumsden self-assigned this Feb 28, 2023

ilumsden force-pushed the ucx branch from 0307c82 to eb83592 Compare March 15, 2023 21:00

ilumsden marked this pull request as ready for review March 15, 2023 21:21

ilumsden marked this pull request as draft March 15, 2023 21:21

ilumsden force-pushed the ucx branch from d6e8961 to eb83592 Compare March 15, 2023 21:22

ilumsden force-pushed the ucx branch from 6832a7e to 669eb3c Compare April 26, 2023 18:32

ilumsden force-pushed the ucx branch from 5ed9909 to 87778d1 Compare May 18, 2023 20:43

ilumsden requested a review from JaeseungYeom May 18, 2023 20:54

ilumsden marked this pull request as ready for review May 18, 2023 20:55

ilumsden force-pushed the ucx branch from 4067771 to e195afc Compare July 6, 2023 19:49

ilumsden force-pushed the ucx branch from ee17831 to c97a23d Compare July 11, 2023 20:06

ilumsden mentioned this pull request Jul 20, 2023

Adds a dedicated Python API #27

Merged

ilumsden force-pushed the ucx branch from c27802b to 5b54cea Compare July 27, 2023 21:22

ilumsden mentioned this pull request Jul 28, 2023

Caliper Integration #39

Closed

JaeseungYeom reviewed Jul 28, 2023

View reviewed changes

src/dtl/dyad_dtl_impl.c Show resolved Hide resolved

JaeseungYeom reviewed Jul 28, 2023

View reviewed changes

src/dtl/dyad_dtl.h Outdated Show resolved Hide resolved

JaeseungYeom reviewed Jul 28, 2023

View reviewed changes

src/dtl/dyad_dtl_impl.c Outdated Show resolved Hide resolved

JaeseungYeom reviewed Jul 28, 2023

View reviewed changes

src/dtl/dyad_dtl_impl.c Outdated Show resolved Hide resolved