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

Generalize and improve hook_args.py implementation #2591

Merged
merged 10 commits into from Sep 9, 2019

Conversation

jvmncs
Copy link
Contributor

@jvmncs jvmncs commented Sep 5, 2019

Note to reviewer: code review will be easiest here if taken commit-by-commit. Care was taken to make sure that the commit messages are comprehensive and descriptive.

The main purpose of this PR is to generalize hook_args to non-torch frameworks. This was actually a very small amount of work and can be seen in the first and third commits in this PR, however I noticed several problems across the code base that I'm also addressing here:

  1. Importing was a struggle after moving files around, because we were often using absolute-ish paths (non-relative paths but importing a lot of things in various init.py files throughout the repo). I've standardized imports to be completely absolute across the repo. This means that you should never import things from an init.py other than the main one in the syft/ package (which should mainly consist of the user-facing API). If there happen to be any leftover exceptions to this, they are only in packages with very low connectivity with their submodules and other modules, and they were left untouched by accident.
  2. After debugging all the import issues related to the above, I realized that the syft.frameworks.torch.hook_args module had quite a few circular dependencies with modules that define their own tensor types. The main reason for this was that hook_args needs these tensor types in its type_rule, forward_func, and backward_func registries, however hook_args was also being used in many of these tensor types' definitions. To fix this, registering tensor types should now happen in the same module as their definition using these new functions.

@review-notebook-app
Copy link

Check out this pull request on ReviewNB: https://app.reviewnb.com/OpenMined/PySyft/pull/2591

You'll be able to see notebook diffs and discuss changes. Powered by ReviewNB.

Copy link
Contributor

@LaRiffle LaRiffle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really a huge move towards a framework generic PySyft!

You made an ambitious change by making all imports absolute, as it allows to get rid of the circular import error, I believe it a great choice. However, it will make it harder for people to import objects in their notebook and so on, so I think we should be generous in syft/init.py to provide many shortcuts just like we can currently call sy.PointerTensor(...). This should include all tensors, workers, and objects a normal user is expected to play with (I believe many of them are already there).

examples/experimental/AutogradTensor.ipynb Outdated Show resolved Hide resolved
syft/frameworks/torch/hook/hook.py Show resolved Hide resolved
syft/workers/base.py Outdated Show resolved Hide resolved
syft/workers/base.py Outdated Show resolved Hide resolved
test/conftest.py Outdated Show resolved Hide resolved

# This import statement is strictly here to trigger registration of syft
# tensor types inside hook_args.py.
import syft.frameworks.torch.hook.hook_args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you did this, but I wonder if there is not a more pythonic way to trigger this registration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open to ideas!

syft/__init__.py Show resolved Hide resolved
syft/federated/train_config.py Show resolved Hide resolved
@jvmncs
Copy link
Contributor Author

jvmncs commented Sep 6, 2019

@LaRiffle

However, it will make it harder for people to import objects in their notebook and so on, so I think we should be generous in syft/init.py to provide many shortcuts just like we can currently call sy.PointerTensor(...). This should include all tensors, workers, and objects a normal user is expected to play with (I believe many of them are already there).

In fact, I don't think many things other than the hook and workers are used in the notebooks. It seems very few tensors actually need to be exposed -- PointerTensor/MultiPointerTensor seem to be abstracted away from the user, and similar for the various precision & crypto tensors.

I'm inclined to remove things that are only exposed for internal development. The more we add to syft/__init__.py, the more complex our import graph gets. That quickly becomes a much bigger headache than having to add a specific import statement for the class I'm using inside of syft internals.

@jvmncs jvmncs dismissed midokura-silvia’s stale review September 6, 2019 17:40

import style ready for discussion in #2599

@jvmncs
Copy link
Contributor Author

jvmncs commented Sep 6, 2019

I'm inclined to remove things that are only exposed for internal development.

Well, unfortunately there are a number of places that rely on tensor being exposed in the syft namespace. I'm taking this to mean that we have a lot more circularity than we want throughout the code (in particular, tensors seem to be highly intertwined). Whether this is okay or not, and whatever we end up doing about it, is almost definitely out of scope. I will keep tensor types in the syft namespace until we figure that out.

Opened a discussion issue around this in #2600

@jvmncs
Copy link
Contributor Author

jvmncs commented Sep 6, 2019

This is ready for another review!

@jvmncs jvmncs requested a review from LaRiffle September 9, 2019 10:52
Copy link
Contributor

@LaRiffle LaRiffle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very few last questions

syft/generic/frameworks/hook/hook_args.py Show resolved Hide resolved
test/test_serde.py Show resolved Hide resolved
@jvmncs jvmncs merged commit 39394c0 into OpenMined:dev Sep 9, 2019
@jvmncs jvmncs deleted the improve-hkargs branch September 9, 2019 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants