Cleanup torch::jit::script::Module API for accessing attributes/parameters/submodules. by ZolotukhinM · Pull Request #27260 · pytorch/pytorch

ZolotukhinM · 2019-10-02T23:42:10Z

Stack from ghstack:

Cleanup torch::jit::script::Module API for accessing attributes/parameters/submodules. #27260 Cleanup torch::jit::script::Module API for accessing attributes/parameters/submodules.

This PR has the following changes:

Slot class is removed. In all use cases except lower_graph we really
just needed the attribute name and thus having an extra layer of
abstraction through Slot only made the code harder to understand.
get_parameters, get_attributes, get_modules, and get_slots now return
a list of <name, item> pairs instead of a list of Slots.

Differential Revision: D17728910

Implement them directly over module-object methods and not use Slots when possible to avoid extra layer of abstraction and make the code more explicit. [ghstack-poisoned]

Implement them directly over module-object methods and not use Slots when possible to avoid extra layer of abstraction and make the code more explicit. ghstack-source-id: 4b2d42f Pull Request resolved: #27260

Implement them directly over module-object methods and not use Slots when possible to avoid extra layer of abstraction and make the code more explicit. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

Implement them directly over module-object methods and not use Slots when possible to avoid extra layer of abstraction and make the code more explicit. ghstack-source-id: 216f482 Pull Request resolved: #27260

zdevito

I have mixed feelings about this because it removes Slot, which was a useful abstraction for getting an LValue for parts of a class/module. I understand it is weird to have them only for modules, but I would hope just promote them into something that works on class objects as well rather than delete them. Eventually, we might want something like my_class.attr("foo").set(ivalue) to work for users.

ZolotukhinM · 2019-10-04T15:50:52Z

Yeah, I actually felt that it was in a half-finished state too, but was interested in the feedback anyway. Let me try to do something to the API of the three classes to see if we can find a better approach.

My main motivation to kill Slot was that it creates an extra layer that actually could be replaced with just functions making the code easier to comprehend. When I first read the code it seemed confusing to me to have both attributes, whose "indexes" are strings, and slots, whose indexes are integers. All in all, it seems that there is a duplicated functionality there and that's what I'm trying to cleanup.

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

zdevito · 2019-10-04T21:35:12Z

There is a real distinction between functions that work on the Type (e.g. get me the slot index for "foo") and those that work on the object (get me the value of "foo"). So they cannot be removed entirely. But I do think we could do a better job making their APIs consistent so the distinction is clear.

ZolotukhinM · 2019-10-04T21:38:09Z

There is a real distinction between functions that work on the Type (e.g. get me the slot index for "foo") and those that work on the object (get me the value of "foo").

That's exactly why I wanted to get rid of Slot which makes you to have both. In all except one use-cases the function worked either on the type or on the object (not both), and thus there really was no need in having Slot around - we could directly work on the type or on the object. The only exception is lower_graph and frankly I think it now needs to be factored out from the Module API into a stand-alone path.

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. ghstack-source-id: 18bba0b Pull Request resolved: #27260

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. ghstack-source-id: 5529d30 Pull Request resolved: #27260

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. ghstack-source-id: d40f0a2 Pull Request resolved: #27260

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

suo

Looks good overall, left some comments inline.

suo · 2019-10-14T23:00:00Z

torch/csrc/jit/script/module.h

-      if (fn->name() == basename) {
-        return Method(module_object(), fn);
-      }
+  c10::optional<size_t> find_parameter(const std::string& name) const;


I don't like this change; it complicates to a common operation (looking up stuff on a module), and it seems like if get_x returns T, find_x should return optional<T> for least surprise.

torch/csrc/jit/script/module.h

suo · 2019-10-14T23:16:16Z

torch/csrc/jit/script/module.h

  size_t num_slots() const {
    return module_object()->slots().size();
  }
+  c10::optional<EntityType> entity_type(const std::string& name) const {


Should these two methods be private? EntityType shouldn't be a public concept I think

Unfortunately, it's now used in slot_iterator_impl, so I had to keep it public. Do you mind if I take a look at how this can be cleaned-up in a follow-up?

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

ZolotukhinM

Updated, please take another look!

ZolotukhinM · 2019-10-15T00:03:20Z

torch/csrc/jit/script/module.h

  size_t num_slots() const {
    return module_object()->slots().size();
  }
+  c10::optional<EntityType> entity_type(const std::string& name) const {


Unfortunately, it's now used in slot_iterator_impl, so I had to keep it public. Do you mind if I take a look at how this can be cleaned-up in a follow-up?

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. Pull Request resolved: #27260 This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910/) ghstack-source-id: 2a65af2

suo · 2019-10-15T02:48:16Z

torch/csrc/jit/script/module.h

 struct slot_list_impl;
-using slot_list = slot_list_impl<Slot>;
-using module_list = slot_list_impl<Module>;
+using module_list = slot_list_impl<std::pair<std::string, Module>>;


My comment here got eaten somehow: this is a readability regression, as the fields name and value have replaced by first and second. I don't think it's a huge deal since first and second are STL convention, but it could be worth retaining a small struct for naming here.

I agree, but do think this is a big regression in understandability of the API. I think use of std::pair and std::tuple are almost always wrong. We should at the very least have a struct with names and not replace a bunch of types with auto in the code.

zdevito

I think this change is fine as long as we replace the std::pair with a struct.

zdevito · 2019-10-15T03:42:02Z

torch/csrc/jit/passes/quantization.cpp

  map[module.module_object()] = qconfig;

-  for (script::Slot s : module.get_module_slots()) {
+  for (auto p : module.get_modules()) {


This code is less understandable than before. It replaced a named type with auto, and replaced informative function names with first and second.

This code is less understandable than before. It replaced a named type with auto, and replaced informative function names with first and second.

Sure, I'll replace pairs with structs.

But that said, I am not completely sold on "code with pairs is less readable than code with a custom arbitrary type". Take into account that you are already familiar with the code, and you know what Slot is - that's why that version is more readable to you. However, for new people (e.g. for me) code with .first and .second is much easier to understand because I immediately know what it means in contrast to a code that uses Slot with 10 methods to operate on its state.

Having a simple structure with no methods seems to be a good choice in the middle though.

zdevito · 2019-10-15T03:44:29Z

torch/csrc/jit/script/module.cpp

-  for (auto submod : get_modules()) {
-    submod.apply(fn);
+  for (auto p : get_modules()) {
+    p.second.apply(fn);


What happened?

zdevito · 2019-10-15T03:46:15Z

torch/csrc/jit/script/module.h

 struct slot_list_impl;
-using slot_list = slot_list_impl<Slot>;
-using module_list = slot_list_impl<Module>;
+using module_list = slot_list_impl<std::pair<std::string, Module>>;


I agree, but do think this is a big regression in understandability of the API. I think use of std::pair and std::tuple are almost always wrong. We should at the very least have a struct with names and not replace a bunch of types with auto in the code.

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. Pull Request resolved: #27260 This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910/) ghstack-source-id: d40a3d5

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. Pull Request resolved: #27260 This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910/) ghstack-source-id: 91969875

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. Pull Request resolved: #27260 This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910/) ghstack-source-id: 907ecb9

…butes/parameters/submodules." This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910) [ghstack-poisoned]

…eters/submodules. Pull Request resolved: #27260 This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: [D17728910](https://our.internmc.facebook.com/intern/diff/D17728910/) ghstack-source-id: d1d2494

…eters/submodules. (#27260) Summary: Pull Request resolved: pytorch/pytorch#27260 This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: D17728910 Test Plan: Imported from OSS Pulled By: ZolotukhinM fbshipit-source-id: 94781611752dd88e7fddfe8b8e0252d6ec32ba68

facebook-github-bot · 2019-10-17T07:11:21Z

@ZolotukhinM merged this pull request in 2265cdd.

…eters/submodules. (pytorch#27260) Summary: Pull Request resolved: pytorch#27260 This PR has the following changes: - Slot class is removed. In all use cases except `lower_graph` we really just needed the attribute name and thus having an extra layer of abstraction through Slot only made the code harder to understand. - get_parameters, get_attributes, get_modules, and get_slots now return a list of <name, item> pairs instead of a list of Slots. Differential Revision: D17728910 Test Plan: Imported from OSS Pulled By: ZolotukhinM fbshipit-source-id: 94781611752dd88e7fddfe8b8e0252d6ec32ba68

Refactor torch::jit::script::Module::find_ methods.

6407988

Implement them directly over module-object methods and not use Slots when possible to avoid extra layer of abstraction and make the code more explicit. [ghstack-poisoned]

ZolotukhinM requested review from apaszke, ebetica, goldsborough and yf225 as code owners October 2, 2019 23:42

pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: cpp Related to C++ API module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries labels Oct 2, 2019

ZolotukhinM mentioned this pull request Oct 2, 2019

Refactor torch::jit::script::Module::register_* API. #27189

Closed

ZolotukhinM requested review from suo and zdevito October 2, 2019 23:47

zdevito reviewed Oct 4, 2019

View reviewed changes

ZolotukhinM changed the title ~~Refactor torch::jit::script::Module::find_ methods.~~ Cleanup torch::jit::script::Module API for accessing attributes/parameters/submodules. Oct 4, 2019

ZolotukhinM requested a review from zdevito October 4, 2019 21:33

suo requested changes Oct 14, 2019

View reviewed changes

ZolotukhinM commented Oct 15, 2019

View reviewed changes

ZolotukhinM requested a review from suo October 15, 2019 00:04

suo approved these changes Oct 15, 2019

View reviewed changes

zdevito requested changes Oct 15, 2019

View reviewed changes

facebook-github-bot closed this in 2265cdd Oct 17, 2019

facebook-github-bot added the merged label Oct 17, 2019

facebook-github-bot deleted the gh/ZolotukhinM/128/head branch October 28, 2019 22:07

mruberry added the Merged label Oct 28, 2020

Conversation

ZolotukhinM commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM commented Oct 4, 2019

Uh oh!

zdevito commented Oct 4, 2019

Uh oh!

ZolotukhinM commented Oct 4, 2019

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZolotukhinM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ZolotukhinM commented Oct 2, 2019 •

edited

Loading