[JIT] Refactor attributes to support buffers and parameters as first class citizens, add support for iterating over `named_buffers()` by voznesenskym · Pull Request #37905 · pytorch/pytorch

voznesenskym · 2020-05-06T02:58:44Z

First part of #36211 - still a WIP, but asking for commentary to ensure this is the direction we want to go in.

dr-ci · 2020-05-06T03:27:20Z

💊 CI failures summary and remediations

As of commit 436f389 (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/py3.6-clang7-rocmdeb-ubuntu16.04

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 126 times.

suo

Some high level thoughts, haven't had a chance to look deeply:

We should probably first tackle the change where we add buffers as a first-class concept on ClassType, separately from adding a named_buffers() iterator.
is_parameter and was_registered_as_buffer are mutually exclusive. Conceivably there should be an enum that expresses the three types of tensor of attributes directly (parameters, buffers, regular attributes).

voznesenskym · 2020-05-06T06:49:06Z

@suo - Good suggestion. Making it a first class citizen + adding an enum would let us have a single collection to manage what we have in each slot on ClassType. I'll go back to the drawing board and look at how we can cleanly manage all 3. Looking at it, perhaps we can remove attributeNames_, attributeTypes_ parameterSlots_, and instead have a single std::vector<AttributeType> where AttributeType contains:

the value for the enum you mentioned
the attribute name (string) (if relevant)
the attributeType (TypePtr)

Thoughts?

suo · 2020-05-06T17:56:34Z

Yeah I think that's a good overall direction. Some minor bits of context that may help guide the data structure decision:

Runtime objects (instances of a ClassType) do not know about names, they store their attributes in a flat vector of IValue slots, to avoid hash table lookups in getattr. So we need a way of efficiently retrieving the slot of something by name on ClassType.
Objects may want to look up name/type/parameter status by slot.
It's okay to trade efficiency of retrieval in ClassType (which is a compile-time construct) for efficiency of retrieval in ivalue::Object (which is run-time). But that might not be relevant for this.

…/refactor_class_type_attr

…m/named_buffers

…pytorch into voznesenskym/named_buffers

…/named_buffers

suo

Nice! This is definitely on the right track. I left a somewhat unusual number of bikeshedding comments—this is a core API so we want to get the naming/concepts consistent.

suo · 2020-05-13T21:38:04Z

+
+  size_t slot = attributes_.size();
+
+  ATTRIBUTE_KIND_ENUM kind = ATTRIBUTE_KIND_ENUM::REGULAR_ATTRIBUTE;


Should we consider just making ATTRIBUTE_KIND_ENUM something that the user sets? Hiding allows some weird behavior. For example: right now you can call addAttribute and have something be both a parameter and buffer—this code would silently mark the attribute as a parameter.

I'd rather keep it as a detail of our system, and then provide safeguards. Users already signal if something is a buffer or parameter by how they register it with our module.

curious what happens in eager if someone registers the same name/value pair as a parameter and buffer…

I'll add a test for this!

suo · 2020-05-13T21:41:56Z

        torch.jit.script(f)

+
+    def named_buffers_are_iterable(self):


I think unittest expects test methods to start with test_?

suo · 2020-05-13T21:45:03Z

      const SourceRange& loc,
      Function& m);

+  std::shared_ptr<SugaredModuleDict> getSugaredNamedBufferDict(


Probably we should rename SugaredModuleDict something more general, to reflact that it is not just being used as for named_modules() anymore.

Agreed. Any suggestions?

SugaredDict?

…/named_buffers

suo

nice, very close!

suo · 2020-05-15T18:03:46Z

+    return attributeType_;
+  }
+
+  const std::string getName() const {


suo · 2020-05-15T18:12:26Z

-    type()->addOrCheckAttribute(name, TensorType::get());
+    bool is_param = false;
+    bool is_buffer = true;
+    type()->addOrCheckAttribute(name, TensorType::get(), is_param, is_buffer);


Hmm…having a lot of boolean defaults in a row is getting a little scary. For example, there is a bug here, because you are actually setting is_param = false, allow_any = true, is_buffer=false, which is not what you want. I think either we should undefault them or change it to something more declarative than several flags

Agreed. I know you said it earlier and I pushed back, but I find myself coming around to the idea of replacing it with our enum.

I like your earlier suggestion of utilizing our enum. Since this would be a large (but relatively simple) change, are you fine with me moving the attribute adding APIs to the new enum in a subsequent diff?

yeah I think that makes sense!

This is still a bug, right?

Yea, I thought I pushed a fix. will update.

suo · 2020-05-15T18:20:28Z

      // attributes and parameters.
      if (is_module) {
        std::vector<std::string> params;
+        std::vector<std::string> buffers;


Nice! Before committing this, we'll have to test for forward compatibility. Basically, can you save() with buffers, then load() using a version of pytorch that does not understand buffer serialization.

Based on skimming the import code, I think there will be an FC issue. Let's chat directly about the steps to take here.

Agreed, let me figure out the best way to do so.

suo · 2020-05-17T03:31:17Z

      at::Tensor v,
      bool is_buffer) {
-    type()->addOrCheckAttribute(name, TensorType::get(), !is_buffer);
+    type()->addOrCheckAttribute(name, TensorType::get(), !is_buffer, is_buffer);


Same here (is_buffer is being passed to allow_any)

Yeah, will fix. Good catch.

suo

looks good! I recommend sanity-checking the FC changes manually if you haven't already.

voznesenskym · 2020-05-19T01:49:27Z

FC tests completed via:

Saving models locally on current commit
Switching branches
Rebuilding pytorch
Loading models on older versions (b9c5375)

facebook-github-bot

@voznesenskym has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-05-19T08:18:53Z

@voznesenskym merged this pull request in f6f1384.

voznesenskym added 7 commits May 5, 2020 13:51

[WIP] [Do not review] [JIT] Add support for using named_buffers

1316884

Merge branch 'master' of github.com:pytorch/pytorch

6e3aae9

Add test, cleanup part 0

898d4c5

Cleanup, part 1

f7ea722

Rebase, fix stuff up

1b22f93

Rebase, fix stuff up

8b49486

cleanup part 2, comments

9133c5a

voznesenskym requested a review from apaszke as a code owner May 6, 2020 02:58

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label May 6, 2020

voznesenskym requested a review from suo May 6, 2020 02:59

voznesenskym changed the title ~~[WIP] [RFC] Add support for iterating over named_buffers()~~ [WIP] [RFC] [JIT] Add support for iterating over named_buffers() May 6, 2020

suo reviewed May 6, 2020

View reviewed changes

voznesenskym and others added 15 commits May 6, 2020 15:04

Linter pass

5ca4d4a

Merge branch 'master' of github.com:pytorch/pytorch into voznesenskym…

1db27fc

…/refactor_class_type_attr

[WIP] Refactor classtype

6f2650b

[WIP] Refactor classtype

18fc401

Fix more type associations

1ec3e82

Merge branch 'voznesenskym/refactor_class_type_attr' into voznesensky…

77242e0

…m/named_buffers

Symbol redup fix, better str

8ac4e1f

[WIP] [RFC] [JIT] Make attributes a first class member of classtype

adf9233

Merge branch 'voznesenskym/named_buffers' of github.com:voznesenskym/…

a8e5aeb

…pytorch into voznesenskym/named_buffers

Run clang_format

34e1160

unset tp changes

6071b78

Fix third party stuff

2f9a364

Merge branch 'master' of github.com:pytorch/pytorch into voznesenskym…

0ab5dc8

…/named_buffers

wip

38db146

Fix up broken tests but putting back a method

50a1d00

voznesenskym added 3 commits May 9, 2020 14:10

Cleanup, change how we store types

011ba40

Fix erasing for parallel structure

e9f3588

Self nits, add checks, comments, remove spurious check

f8b607d

voznesenskym changed the title ~~[WIP] [RFC] [JIT] Add support for iterating over named_buffers()~~ [JIT] Refactor attributes to support buffers and parameters as first class citizens, add support for iterating over named_buffers() May 10, 2020

voznesenskym requested a review from suo May 13, 2020 05:03

suo reviewed May 13, 2020

View reviewed changes

Comment thread aten/src/ATen/core/jit_type.h Resolved

suo requested changes May 13, 2020

View reviewed changes

voznesenskym added 5 commits May 13, 2020 15:49

Feedback first pass

b1b4958

Feedback, fix test, make serialization work

49868d5

Merge branch 'master' of github.com:pytorch/pytorch into voznesenskym…

d1d8e6e

…/named_buffers

Run linter, fix flake8

f01e554

Make it work with the legacy executor

9023488

suo requested changes May 15, 2020

View reviewed changes

suo linked an issue May 15, 2020 that may be closed by this pull request

[JIT] support self.named_buffers and self.named_parameters in TorchScript #36211

Closed

voznesenskym added 3 commits May 15, 2020 15:02

Forward Compatability for FBCode

4300549

Clang format

1d36602

Nits

2300421

suo reviewed May 17, 2020

View reviewed changes

voznesenskym added 2 commits May 16, 2020 21:11

Great @suo catch on new api default mistake by me

f99cfc0

once more - linter pass

436f389

suo approved these changes May 18, 2020

View reviewed changes

facebook-github-bot reviewed May 19, 2020

View reviewed changes

facebook-github-bot closed this in f6f1384 May 19, 2020

facebook-github-bot added the merged label May 19, 2020

ppwwyyxx mentioned this pull request Jun 14, 2020

Make anchor_generator scriptable facebookresearch/detectron2#1518

Closed

mruberry added the Merged label Oct 28, 2020


		size_t slot = attributes_.size();

		ATTRIBUTE_KIND_ENUM kind = ATTRIBUTE_KIND_ENUM::REGULAR_ATTRIBUTE;

Conversation

voznesenskym commented May 6, 2020

Uh oh!

dr-ci Bot commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

voznesenskym commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suo commented May 6, 2020

Uh oh!

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

suo May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

voznesenskym commented May 19, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 19, 2020

dr-ci Bot commented May 6, 2020 •

edited

Loading

voznesenskym commented May 6, 2020 •

edited

Loading

suo May 17, 2020 •

edited

Loading