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.

Sign up for GitHub

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

Jump to bottom

Get ExecGraph hosts #178

Merged
merged 5 commits into from
Nov 23, 2021
Merged

Get ExecGraph hosts #178

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7fb7eab
add utility function to return the set of distinct hosts in an exec g…
csegarragonz Nov 22, 2021
142e513
bump version
csegarragonz Nov 22, 2021
ee278b7
refactor from count to get
csegarragonz Nov 22, 2021
d21fb97
pr comments
csegarragonz Nov 22, 2021
aebd37a
bump faabric base
csegarragonz Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
FAABRIC_VERSION=0.2.1
FAABRIC_CLI_IMAGE=faasm/faabric:0.2.1
FAABRIC_VERSION=0.2.2
FAABRIC_CLI_IMAGE=faasm/faabric:0.2.2
COMPOSE_PROJECT_NAME=faabric-dev
6 changes: 3 additions & 3 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
REDIS_QUEUE_HOST: redis
REDIS_STATE_HOST: redis
container:
image: faasm/faabric:0.2.1
image: faasm/faabric:0.2.2
defaults:
run:
working-directory: /code/faabric
Expand Down Expand Up @@ -45,7 +45,7 @@ jobs:
REDIS_QUEUE_HOST: redis
REDIS_STATE_HOST: redis
container:
image: faasm/faabric:0.2.1
image: faasm/faabric:0.2.2
defaults:
run:
working-directory: /code/faabric
Expand Down Expand Up @@ -92,7 +92,7 @@ jobs:
REDIS_QUEUE_HOST: redis
REDIS_STATE_HOST: redis
container:
image: faasm/faabric:0.2.1
image: faasm/faabric:0.2.2
defaults:
run:
working-directory: /code/faabric
Expand Down
2 changes: 1 addition & 1 deletion VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.2.1
0.2.2
5 changes: 5 additions & 0 deletions include/faabric/scheduler/ExecGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
#pragma once

#include <faabric/proto/faabric.pb.h>

#include <set>
#include <vector>

namespace faabric::scheduler {

struct ExecGraphNode
{
faabric::Message msg;
Expand All @@ -17,6 +20,8 @@ struct ExecGraph

int countExecGraphNodes(const ExecGraph& graph);

std::set<std::string> getExecGraphHosts(const ExecGraph& graph);

std::string execNodeToJson(const ExecGraphNode& node);

std::string execGraphToJson(const ExecGraph& graph);
Expand Down
2 changes: 1 addition & 1 deletion mpi-native/mpi-native.env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FAABRIC_VERSION=0.2.1
FAABRIC_VERSION=0.2.2
FAABRIC_MPI_NATIVE_IMAGE=faasm/faabric-mpi-native:0.0.18
COMPOSE_PROJECT_NAME=faabric-mpi

Expand Down
22 changes: 22 additions & 0 deletions src/scheduler/ExecGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,28 @@ int countExecGraphNodes(const ExecGraph& graph)
return count;
}

std::set<std::string> getExecGraphHostsForNode(const ExecGraphNode& node)
{
std::set<std::string> hostsForNode;
hostsForNode.insert(node.msg.executedhost());

if (!node.children.empty()) {
csegarragonz marked this conversation as resolved.
Show resolved Hide resolved
for (auto c : node.children) {
std::set<std::string> nodeHost = getExecGraphHostsForNode(c);
hostsForNode.insert(nodeHost.begin(), nodeHost.end());
}
}

return hostsForNode;
}

std::set<std::string> getExecGraphHosts(const ExecGraph& graph)
{
ExecGraphNode rootNode = graph.rootNode;
std::set<std::string> hosts = getExecGraphHostsForNode(rootNode);
return hosts;
csegarragonz marked this conversation as resolved.
Show resolved Hide resolved
}

// ----------------------------------------
// TODO - do this with RapidJson and not sstream
// ----------------------------------------
Expand Down
19 changes: 19 additions & 0 deletions tests/test/scheduler/test_exec_graph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,25 @@ TEST_CASE("Test execution graph", "[scheduler][exec-graph]")
checkExecGraphEquality(expected, actual);
}

TEST_CASE("Test execution graph node count", "[scheduler][exec-graph]")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very nitpicky but the API is about getting the unique host names from the graph, not the count.

I'd have thought this test would be called Test get unique hosts from exec graph, then the assertion at the end would look like:

std::set<std::string> expected = {"foo", "bar", "baz"};
REQUIRE(hosts == expected);

Currently the test also doesn't cover duplicates, it's got 3 messages each executed on a different host. It would be good to add another message for a different function but executed on the same host as one of the others to be extra sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good points, thanks.

{
faabric::Message msgA = faabric::util::messageFactory("demo", "echo");
faabric::Message msgB1 = faabric::util::messageFactory("demo", "echo");
faabric::Message msgB2 = faabric::util::messageFactory("demo", "echo");

msgA.set_executedhost("foo");
msgB1.set_executedhost("bar");
msgB2.set_executedhost("baz");

ExecGraphNode nodeB2 = { .msg = msgB2 };
ExecGraphNode nodeB1 = { .msg = msgB1 };
ExecGraphNode nodeA = { .msg = msgA, .children = { nodeB1, nodeB2 } };

ExecGraph graph{ .rootNode = nodeA };
auto hosts = faabric::scheduler::getExecGraphHosts(graph);
REQUIRE(hosts.size() == 3);
}

TEST_CASE_METHOD(MpiBaseTestFixture,
"Test MPI execution graph",
"[mpi][scheduler][exec-graph]")
Expand Down