Communication job

[kiddyboots216]

This PR currently

• Adds a Communication Job to the Runner by utilizing the setter in blockchain_utils
• Adds support for JOB_COMM to the Optimizer, where previously JOB_COMM was just a dummy job
• Reimplements JOB_AVG because nobody would let me put JOB_COMM with JOB_AVG so the ENTIRE AVERAGING PR was wrong. LOL.
• Integrates communication with support for JOB_AVG to the Optimizer by communicating an entire job rather than just weights (sparse amount of metadata is added and the tradeoff in usability/extendability is well worth it)
• Uses TxEnum everywhere to support integration with blockchain rather than hardcoded values
• Adds tests for above functionality to fed_avg_optimizer, runner, and scheduler
• Adds Blockchain Gateway to Bootstrapper in the same way that Scheduler is used in Bootstrapper
• Adds an integration testing file which comprehensively tests federated learning between 2 clients (automated by mimicking the bootstrapper), fed. learning between 2 clients (manual), and the communication manager (more in-depth and shows what's going on behind the scenes)
• Refactors the Scheduler to be threadsafe/process-safe
• Refactors pytest fixtures to use (scope=session) when possible
• Adds an IPFS client to the bootstrapper so it can be configured into the Runner/Dataset Manager/Blockchain Gateway at initialization

[kiddyboots216]

"I would consider keeping the .travis.yml comments. They're useful." You...literally asked them to be removed in the Google doc...but sure.

"Since this PR is adding the Blockchain Gateway to the bootstrapper, can we have instructions on how to run the dml command locally and tinker with the BG? (Ideally we're able to replicate the demo you did during the last meeting.)" The instructions are already in the README on Master.

"Look into removing set_weights and set_keys in the DMLJob." Definitely should do this.

"Explain the rationale of adding do_nothing(), NEW_SESSION, and TERMINATE to the Optimizer." Rationale is already explained in the docstring.

"Can we do some error checking in the runner's _communicate() routine? What if the setter doesn't return a valid tx_receipt? Should that instead return a failed DMLResult?" tx_receipt contains errors in consensus, as for actual errors in the service that needs to be handled with the general changes to Runner error-handling. So I don't think so.

"Make L63 of the integration tests more explicit." Can you elaborate?

Everything else addressed with the exception of scheduling communication_job, which as it turns out doesn't work very well. I've tried debugging this for a while but can't make any headway.

[georgymh]

The instructions are already in the README on Master.

It's incomplete though. You should add instructions/snippets to submit DML requests through the Blockchain Gateway after running dml.

Rationale is already explained in the docstring.

I checked again and didn't see any rationale. I will just answer it here for documentation purposes (and so that other reviewers are aware of the whys):

• NEW_SESSION is added because the Blockchain Gateway may pick up a new session message in the middle of a session and we want the service to just ignore it. Hence the _do_nothing().

• TERMINATE was just an addition we were already planning to do. The Blockchain Gateway may pick up a termination message, in which case the Optimizer will instruct the Communication Manager to end the session (and destroy the Optimizer).

"Make L63 of the integration tests more explicit." Can you elaborate?

Use something like:

function(arg1=a, arg2=b, arg3=c)

But properly indented.

[kiddyboots216]

There is no such thing as "submitting DML requests through the Blockchain Gateway after running DML". Explora is the only interface for users to submit anything to the blockchain. The DML Service doesn't expose any such interface. Addressed your other comment.

[georgymh]

We should be able to mock that, no? Is there a way to have only one node in the network and do the same HTTP request that explora would be doing to set something on the blockchain?

I think on your demo you were starting with a blockchain that already had a job/DML request in it. That would work too. The point is to allow us to do more than just run dml and patiently wait for nothing.

[kiddyboots216]

The easiest way is to create a pytest which sets exactly what you want to the chain, run that pytest only and then start the DML service without resetting the chain. I can make the process easier after the next blockchain PR is merged but it's not really feasible to add to this PR.

[georgymh]

Rather than a test, could we have a utils we could execute from command line? I'm also ok with calling curl manually. We just need instructions on the README.

[kiddyboots216]

No, it's not feasible to do it from command line. You would have to make the model_json, make a DMLJob, serialize the job, make a session, put the session into ipfs, get the content hash, and then put that onto the blockchain. Calling curl is doable, sure, but the blockchain is going to change anyway, so this is not a good use of time for this PR. Anyways I've updated the README with instructions for this, I suppose.

[georgymh]

Ok, seems like you ended up updating the README anyways.

What's <your DML session here>? Can you put an example DML session there? Or make the function calls necessary to create one?

[georgymh]

Cool. Thanks for "wasting" your time in 2578551 as a way to avoid the team actually wasting its collective time trying to make code they didn't write work. In fact, let's "waste" our time more often.

On a more serious note though, a few of the points I put above weren't addressed (like the travis file one, the .gitignore one, removing the DMLJob functions/explaining why they should be there, and adding docstrings for the new properties in the DMLJob constructor). Please address them and I will take a look at the tests tomorrow. They look comprehensive, which is good.

[neeleshdodda44]

Good work. Left some comments, I'll review again after you address those.

Please try to keep PRs smaller in the future! (195 commits before I started reviewing :( )

[kiddyboots216]

Fixed the bug with Scheduler; it was using collections.deque which is not multiprocessing-safe or thread-safe.