Add location-set as a generic type to Transport #14

ihaveint · 2023-09-18T11:14:45Z

This adds location-set as a generic type to transport, and removes the need for users to specify the location-set for Projector. Now, the location-set of Projector is inferred from the location-set of transport.

shumbo · 2023-09-18T19:19:30Z

chorus_lib/src/core.rs

@@ -338,13 +333,16 @@ where
    where
        L: Subset<LS, IndexSet>,
    {
-        struct EppOp<'a, L: HList, L1: ChoreographyLocation, B: Transport> {
+        struct EppOp<'a, L: HList, L1: ChoreographyLocation, LS: HList, B: Transport<LS>> {


Question: What's the difference between L and LS?

Hmm; It seems to me that L here comes from the Choreography: Look at Line 345. The L first is inferred from the Choreography, then at line 455 we use the same L as a concrete value for the L at line 352.

But LS is the location set for the transport (and the projector). So we need it as a different generic and can't use L. L is a subset of LS (line 350).

There is an issue where LS is first introduced on line 303, but has to be introduced again on line 359; and it seems it's a Rust thing where you can't use a generic of an outer function inside an inner function!

shumbo · 2023-09-18T19:20:18Z

chorus_lib/src/transport/http.rs

@@ -3,6 +3,8 @@
 use std::thread;
 use std::{collections::HashMap, sync::Arc};

+use core::marker::PhantomData;


Should be imported from std::marker

…rtChannel

Allow the Transport to have a different kind of information for the target ChoreographyLocation

…tern

chorus_lib/src/core.rs

chorus_lib/src/transport.rs

shumbo · 2023-09-22T04:31:54Z

chorus_lib/src/transport.rs

+{
+    /// Adds information about a new `ChoreographyLocation`.
+    pub fn with<NewLocation: ChoreographyLocation>(
+        self,


if you take mut self, can you reuse the hashmap?

This is already changed!

chorus_lib/src/transport/http.rs

chorus_lib/src/transport/local.rs

chorus_book/src/guide-transport.md

shumbo · 2023-09-28T05:34:10Z

chorus_book/src/guide-transport.md

 ```

-You can construct a `LocalTransport` instance by passing a slice of locations to the `from` method.
+To use the `local` transport, first import the `LocalTransport` struct from the `chorus_lib` crate.


This sentence seems redundant.

Well, we have this sentence in the current version of the book: "To use the local transport, import the LocalTransport struct from the chorus_lib crate."
In this commit, it's just in a different paragraph. But if you do think that it shouldn't have been there in the first place, sure! I'll delete it (But let me know if you want it to be gone completely or you want me to put it before the snippet)

chorus_book/src/guide-transport.md

chorus_lib/src/transport/http.rs

shumbo · 2023-09-28T05:50:56Z

chorus_lib/src/transport/local.rs

+    pub fn with<NewLocation: ChoreographyLocation>(
+        self,
+        _location: NewLocation,
+    ) -> LocalTransportChannel<HCons<NewLocation, L>> {
        let mut queue_map: QueueMap = HashMap::new();


I know it's not a big deal, but you can probably take mut self and reuse the hashmap without creating a new one all over every time we call with

queue_map is always an Arc, and if you want it to be muted, I think you have to change the type to something else. And even if you can do that, the code will become harder to understand. (Here, it's clear what queue_map contains if someone reads the code, but if you want it to be changed so that only new information is added to it, someone might need to read it a couple of times before they understand all the content, not just how it's updated every time)

This is a library and that complexity won't be exposed to the user. IMO that readability issue is not a valid explanation for creating a nested hash map.

shumbo · 2023-09-28T05:51:50Z

chorus_lib/src/transport/local.rs

+///
+/// This transport uses a blocking queue to allow for communication between threads. Each location must be executed in its thread.
+///
+/// Unlike network-based transports, all locations must share the same `LocalTransport` instance. The struct implements `Clone` so that it can be shared across threads.


I think this comment is no longer true?

Suggested change

/// Unlike network-based transports, all locations must share the same `LocalTransport` instance. The struct implements `Clone` so that it can be shared across threads.

/// Unlike network-based transports, all locations must share the same `LocalTransportChannel` instance.

I do agree with your point, BUT: LocalTransportChannel is not a field in network-based transports, so it doesn't even make sense to compare the two. Can we just go with "all locations must share the same LocalTransportChannel instance" and remove the Unlike network-based transports part?

shumbo · 2023-09-28T06:05:34Z

chorus_lib/src/transport.rs

+    /// The information about the target choreography
+    pub target_info: (TargetLocation, TargetInfoType),
+    /// The struct is parametrized by the location set (`L`).
+    pub location_set: PhantomData<L>,


You probably don't want to pub this

do I make all the fields private? It works currently

No, we need to have other fields public so when people use it for their custom transport they can access the fields

shumbo · 2023-09-28T06:05:45Z

chorus_lib/src/transport.rs

+{
+    /// Adds information about a new `ChoreographyLocation`.
+    pub fn with<NewLocation: ChoreographyLocation>(
+        self,


shumbo · 2023-09-28T06:08:02Z

chorus_lib/src/transport/http.rs

 /// The header name for the source location.
 const HEADER_SRC: &str = "X-CHORUS-SOURCE";

+/// A wrapper for HashMap<String, (String, u16)>


Is this used?

…annel

shumbo · 2023-09-30T16:17:20Z

Merging this PR and I will make more adjustments at #12

ihaveint force-pushed the transport-location-set branch from f42fb67 to 44c3478 Compare September 18, 2023 11:18

Add location-set as a generic type to Transport

2bd32d5

ihaveint force-pushed the transport-location-set branch from 44c3478 to 2bd32d5 Compare September 18, 2023 11:50

Change the book examples. The text itself still needs to be updated

24ad199

ihaveint force-pushed the transport-location-set branch from a1ce6bb to 24ad199 Compare September 18, 2023 11:51

ihaveint added 2 commits September 18, 2023 05:04

Update the book

3fee760

Refactor HttpConfig

140e431

shumbo reviewed Sep 18, 2023

View reviewed changes

ihaveint added 13 commits September 18, 2023 12:53

Fix import for PhantomData

f992cf0

Simplify User-API for building TransportConfig

c4feb9c

WIP: program doesn't terminate

642b971

WIP: the library tests work

e486a7d

Simplify things a bit

5df2e94

Remove redundant target-location argument

d7e593c

Fix some warnings and remove an extra layer of Arc

d835717

Use Equal trait instead of assuming equality of the location sets

65096dd

Update examples

9db4537

Remove the TransportChannel from HttpTransport

1f42d17

Move TransportChannel to LocalTransport and rename it to LocalTranspo…

1739b1b

…rtChannel

Update examples and the book

07f5021

Merge pull request #15 from lsd-ucsc/target-specific-info

5bf932a

Allow the Transport to have a different kind of information for the target ChoreographyLocation

ihaveint requested a review from shumbo September 20, 2023 00:14

Use std::marker::PhantomData instead of core::marker::PhantomData

ab005b2

ihaveint marked this pull request as ready for review September 20, 2023 02:31

Change some imports

9c99e5c

ihaveint force-pushed the transport-location-set branch from cb93c5b to 9c99e5c Compare September 20, 2023 10:08

ihaveint added 2 commits September 20, 2023 23:07

Change the API for building a TransportConfig; now we use builder pat…

0b91d60

…tern

Change the examples and the book to reflect the new API

62f4a59

ihaveint force-pushed the transport-location-set branch from 726e5d0 to 00e6f1c Compare September 21, 2023 12:58

Remove a bunch of unnecessary Arcs

b5dfba0

ihaveint force-pushed the transport-location-set branch 2 times, most recently from 31eaf1a to 8a63b03 Compare September 21, 2023 23:37

Update the guide for Transport

2b379cd

ihaveint force-pushed the transport-location-set branch from 8a63b03 to 2b379cd Compare September 21, 2023 23:42

ihaveint added 2 commits September 21, 2023 16:45

Remove some empty spaces in the book

2597d37

Remove redundant comments

f0b8461

ihaveint force-pushed the transport-location-set branch from 7cae423 to f0b8461 Compare September 21, 2023 23:50

shumbo requested changes Sep 22, 2023

View reviewed changes

ihaveint force-pushed the transport-location-set branch 2 times, most recently from 6948989 to 82e3c07 Compare September 28, 2023 02:23

shumbo reviewed Sep 28, 2023

View reviewed changes

chorus_book/src/guide-transport.md Outdated Show resolved Hide resolved

shumbo requested changes Sep 28, 2023

View reviewed changes

ihaveint force-pushed the transport-location-set branch 2 times, most recently from c2f1eca to a1d2df1 Compare September 29, 2023 10:34

Change the API so that we have a builder pattern for LocalTransportCh…

83a5212

…annel

ihaveint force-pushed the transport-location-set branch from a1d2df1 to 83a5212 Compare September 29, 2023 11:07

shumbo merged commit 6509a2c into shumbo/available-locations Sep 30, 2023
3 checks passed

shumbo mentioned this pull request Oct 1, 2023

Location set tweaks #16

Merged

shumbo deleted the transport-location-set branch October 1, 2023 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add location-set as a generic type to Transport #14

Add location-set as a generic type to Transport #14

ihaveint commented Sep 18, 2023

shumbo Sep 18, 2023

ihaveint Sep 18, 2023 •

edited

Loading

shumbo Sep 18, 2023

shumbo Sep 22, 2023

shumbo Sep 28, 2023

ihaveint Sep 29, 2023

shumbo Sep 28, 2023

ihaveint Sep 29, 2023 •

edited

Loading

shumbo Sep 28, 2023

ihaveint Sep 29, 2023 •

edited

Loading

shumbo Sep 30, 2023

shumbo Sep 28, 2023

ihaveint Sep 29, 2023 •

edited

Loading

shumbo Sep 28, 2023

ihaveint Sep 29, 2023

shumbo Sep 30, 2023

shumbo Sep 28, 2023

shumbo Sep 28, 2023

shumbo commented Sep 30, 2023

	/// Unlike network-based transports, all locations must share the same `LocalTransport` instance. The struct implements `Clone` so that it can be shared across threads.
	/// Unlike network-based transports, all locations must share the same `LocalTransportChannel` instance.

Add location-set as a generic type to Transport #14

Add location-set as a generic type to Transport #14

Conversation

ihaveint commented Sep 18, 2023

Choose a reason for hiding this comment

ihaveint Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihaveint Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihaveint Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihaveint Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shumbo commented Sep 30, 2023

ihaveint Sep 18, 2023 •

edited

Loading

ihaveint Sep 29, 2023 •

edited

Loading

ihaveint Sep 29, 2023 •

edited

Loading

ihaveint Sep 29, 2023 •

edited

Loading