Ported RecommendationSystemTest to EE2. #3276
Conversation
There was a problem hiding this comment.
@gcatron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -745,40 +761,46 @@ class RecommendationSystemTest : public BackendTest { | |||
| /// Create partitions to run and compare results. | |||
| void runPartitionedGraph(size_t numDevices, size_t memSize, | |||
There was a problem hiding this comment.
Do we still need this? Can we just add it to the EE and let it do its thing?
For context this was added originally because we didn't have a nice wrapper of the runtime to test partitioning with, and now we do.
There was a problem hiding this comment.
We still do, this function calls partitioner directly for finer grained testing. That's not something the EE exposes. An alternative would be to go straight to HostManager and bypass EE but that would require its own supporting code.
There was a problem hiding this comment.
@gcatron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -45,6 +45,9 @@ void ExecutionEngine2::setBackendName(llvm::StringRef backend) { | |||
| if (backend != "") { | |||
There was a problem hiding this comment.
what's the use case when backend passed here is actually an empty string?
There was a problem hiding this comment.
You ask a good question. It's currently called in ~ExecutionEngine2, but the module without a HM doesn't make much sense. A follow up Diff to refactor and simplify seems like a good idea.
There was a problem hiding this comment.
Made #3280 to track it. I'll refactor when I have fewer branches to rebase :)
There was a problem hiding this comment.
@gcatron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@gcatron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: This PR ports the RecommendationSystemTest to the new EE2. Additionally this adds a new feature to EE2, the ability to set the memory of the device. This allows programmatically setting the EE device memory to support graphs larger than the default.
Documentation: NA
Work on #3239
Test Plan: verify everything builds, and RecomendationSystemTest runs and passes.