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.

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

Check dimensions of input tensors before executing plans #167

Open
sachin-101 opened this issue Jun 19, 2020 · 5 comments
Open

Check dimensions of input tensors before executing plans #167

sachin-101 opened this issue Jun 19, 2020 · 5 comments
Labels
Priority: 3 - Medium 😒 Should be fixed soon, but there may be other pressing matters that come first Severity: 3 - Medium 😒 Does not cause a failure, impair usability, or interfere with the system Status: Available 👋 Available for assignment, who wants it? Type: Improvement 📈 Performance improvement not introducing a new feature or requiring a major refactor
Milestone

Comments

@sachin-101
Copy link

Description

// Execute the plan and get updated model params back.
let [loss, acc, ...updatedModelParams] = await job.plans[
    'training_plan'
].execute(
    job.worker,
    tf.zeros([chunkSize, 50*120]),
    tf.zeros([chunkSize, 2]),
    chunkSize,
    lr,
    ...modelParams
);            

If the dimensions of the input tensors in the above code snippet are wrong, then syft.js throws the following error
Error: We cannot find function mul in TensorFlow.js, performing a manual lookup..
This error message is not very intuitive.

Instead, we can check for the dimensions of the input tensors, and throw error warning the user regarding the wrong dimension.

Are you interested in working on this improvement yourself?

  • Yes, I am.
@sachin-101 sachin-101 added the Type: Improvement 📈 Performance improvement not introducing a new feature or requiring a major refactor label Jun 19, 2020
@cereallarceny
Copy link
Member

Good find @sachin-101! Would you like to work on this and make a PR? If so, please let me know.

@cereallarceny cereallarceny added Priority: 3 - Medium 😒 Should be fixed soon, but there may be other pressing matters that come first Severity: 3 - Medium 😒 Does not cause a failure, impair usability, or interfere with the system Status: Available 👋 Available for assignment, who wants it? labels Jun 19, 2020
@sachin-101
Copy link
Author

Yes @cereallarceny

@vvmnnnkv
Copy link
Member

vvmnnnkv commented Jun 22, 2020

Can we split into 2 issues?

  1. [fix] Propagate TFJS error when it occurs during plan execution. Right now it's overwritten with translation error because we don't differentiate between TFJS error and Threepio error.
  2. [improvement] check inputs shape. shapes can be obtrained from Plan.input_placeholders, the Placeholder should have expected_shape property. I'm not sure what to do with the batch size dimension. Ideally, PySyft should record it as -1 or None or similar, maybe it's already possible via @func2plan(args_shape=...), needs check.

@sachin-101
Copy link
Author

@vvmnnnkv Go ahead. Shall I modify this issue to only contain the 2'nd part? While you can open the first one in threepio?

@vvmnnnkv
Copy link
Member

Done: #171. Note, it's not threepio problem.

@cereallarceny cereallarceny modified the milestones: 0.1.0, 0.2.0 Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 3 - Medium 😒 Should be fixed soon, but there may be other pressing matters that come first Severity: 3 - Medium 😒 Does not cause a failure, impair usability, or interfere with the system Status: Available 👋 Available for assignment, who wants it? Type: Improvement 📈 Performance improvement not introducing a new feature or requiring a major refactor
Projects
None yet
Development

No branches or pull requests

3 participants