Closed kiddyboots216 closed 5 years ago
Minor things:
Can we return ActionableEventTypes.NOTHING.name, None instead of ActionableEventTypes.NOTHING.name, self.job when appropriate within the Optimizer?
Your changes in the runner seem correct. Do you want to just remove the transform_function properrt from the DMLJob and the tests to complete Neel's Patch PR here? (Otherwise expect to resolve merge conflicts; we can do this here because it's short.)
Can you rename ActionableEventTypes.SCHEDULE_JOB to its plural form?
FYI, within the Optimizer's kickoff method, you could have just used a copy constructor for the 2 DMLJob you created for simplicity.
The changes you made to the gitignore are OK, but there's a cleaner way to prevent those files from being committed and that's just cleaning them up after the tests. Could you do that instead?
I didn't thoroughly check the test files changed by this PR but besides what I mentioned above, I think the other files look good. Neel, if you're able to, please review the tests for Panda.
@georgymh if you can approve the patch or give all necessary by tmrw 4:30 PM that would be ideal, as I'll have another few hours to work tomorrow and if this is merged I can get communication-job ready for review.
With the last point I meant automating the clean up within the tests.
I have some time after class. I will try.
This PR integrates TRANSFORM_SPLIT into the Fed. Avg. Optimizer by updating the Optimizer to return an array of Jobs, updating the Communication Manager to run that array of Jobs, and updating all the tests so that they are ok with this new behavior. It's worth noting that the transform_function absolutely should not be on master. But that should be addressed in a separate PR if and when we make the decision to remove it; for now, I've added a few lines in _transform_and_split_job in the Runner to handle that.