Use latest ExecGraph features and only upon request
#537
Conversation
csegarragonz
changed the title
ExecGraph features and only upon request
| msg = { | ||
| "user": user, | ||
| "function": func, | ||
| "user": "", |
There was a problem hiding this comment.
Passing None here broke the JSON parser in faabric.
| if (originalCall->recordexecgraph()) { | ||
| msg.set_recordexecgraph(true); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we need tests for both setting this to true in chained messages (should be adding a line to an existing test), and the call to logChainedFunction only when this is on? Are they already covered somewhere? What about the case of testing that they're not recorded by default?
As a general rule of thumb, any PR changing C++ code should have a test unless: a) it's already tested; b) it's not possible to test the change. If you're fixing a bug, you should be writing a test that would have caught it in the first place.
| #include <faabric/runner/FaabricMain.h> | ||
| #include <faabric/scheduler/ExecGraph.h> | ||
| #include <faabric/scheduler/Scheduler.h> | ||
| // DELETE MEEEE |
There was a problem hiding this comment.
Do we need to do what it says?
There was a problem hiding this comment.
I completely missed this, sorry.
| #include <faaslet/Faaslet.h> | ||
|
|
||
| namespace tests { | ||
| class ExecGraphTestFixture : public FunctionExecTestFixture |
There was a problem hiding this comment.
We already have tests for the execution graph in tests/test/scheduler/test_exec_graph.cpp, I think these can just be put in there can't they? The exec graph doesn't really have any thing to do with the system module.
There was a problem hiding this comment.
Those tests live in faabric, and in faabric we already check that if the flag is not set, the execution graph is not generated.
I thought your comment in the previous review meant that you wanted to have an e2e check in faasm. Otherwise, I am not sure I understand what you ask for here.
Lastly, I agree that system is not the best place to put this test. I just was not sure where to put it. Let's discuss where to place it offline.
There was a problem hiding this comment.
Gah sorry I'm getting confused now 馃樀, you're right, difficult to merge these tests with those in a different repo 馃槀 .
However, I still think we can find a better home for them. I think the first generic one can go into tests/test/faaslet/test_chaining.cpp, then the MPI one into tests/test/faaslet/test_mpi.cpp.
| ExecGraphTestFixture() | ||
| : m(std::make_shared<faaslet::FaasletFactory>()) | ||
| { | ||
| // Modify system config (network ns requires root) |
There was a problem hiding this comment.
I'm not sure we need this fixture, can we reuse one we already have (e.g. FunctionExecTestFixture)?
The callFunctionGetResult method seems unnecessary as the result isn't used AFAICT. Can we just use execFunction?
This comment also doesn't seem relevant, is it a copy-paste error?
There was a problem hiding this comment.
You are right, the fixture was unnecessary. The call to sch.callFunction is necessary to update the exec graph through faabric (thus we can't use execFunction which invokes the function directly on the module).
| #include <faaslet/Faaslet.h> | ||
|
|
||
| namespace tests { | ||
| class ExecGraphTestFixture : public FunctionExecTestFixture |
There was a problem hiding this comment.
Gah sorry I'm getting confused now 馃樀, you're right, difficult to merge these tests with those in a different repo 馃槀 .
However, I still think we can find a better home for them. I think the first generic one can go into tests/test/faaslet/test_chaining.cpp, then the MPI one into tests/test/faaslet/test_mpi.cpp.
In this PR I bump the faabric dependency to catch (1) the new exec graph details (faasm/faabric#166) and (2) the bug-fixes for the new exec graph details (faasm/faabric#173) 馃ぃ
I also make recording execution graphs an opt-in feature that a user can enable with the
--graphflag on anyinv invokecall.I bump the code version to make it available in production environments.