Operator developer guidance #72470

cccclai · 2022-02-07T22:05:12Z

Stack from ghstack:

-> Operator developer guidance #72470

Use markdown to write an upgrader guidance

Differential Revision: D34054964

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) [ghstack-poisoned]

facebook-github-bot · 2022-02-07T22:05:15Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/72470
📄 Preview docs built from this PR
📄 Preview C++ docs built from this PR
↩️ [fb-only] Re-run with SSH instructions
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

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

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

linux-xenial-py3.7-clang7-onnx / test (default, 1, 2, linux.2xlarge) (1/1)

Step: "Test" (full log | diagnosis details | 🔁 rerun)

2022-02-14T19:31:22.0264386Z �[31mERROR: pip's ... the source of the following dependency conflicts.

2022-02-14T19:31:20.4328918Z ++ stat --format %U /opt/conda/bin/pip
2022-02-14T19:31:20.4339228Z + PIP_USER=jenkins
2022-02-14T19:31:20.4341655Z ++ id -u -n
2022-02-14T19:31:20.4351337Z + CURRENT_USER=jenkins
2022-02-14T19:31:20.4351570Z + [[ jenkins = root ]]
2022-02-14T19:31:20.4351869Z + pip -q uninstall -y hypothesis
2022-02-14T19:31:20.8198319Z + pip -q uninstall -y coverage
2022-02-14T19:31:21.1191167Z �[33mWARNING: Skipping coverage as it is not installed.�[0m
2022-02-14T19:31:21.1622160Z + pip -q install attrs==18.1.0 -f https://s3.amazonaws.com/ossci-linux/wheels/attrs-18.1.0-py2.py3-none-any.whl
2022-02-14T19:31:21.5062468Z �[33mWARNING: Skipping page https://s3.amazonaws.com/ossci-linux/wheels/attrs-18.1.0-py2.py3-none-any.whl because the HEAD request got Content-Type: binary/octet-stream.The only supported Content-Type is text/html�[0m
2022-02-14T19:31:22.0264386Z �[31mERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
2022-02-14T19:31:22.0265021Z pytest 7.0.1 requires attrs>=19.2.0, but you have attrs 18.1.0 which is incompatible.�[0m
2022-02-14T19:31:22.0853034Z + pip -q install coverage==4.5.1 -f https://s3.amazonaws.com/ossci-linux/wheels/coverage-4.5.1-cp36-cp36m-macosx_10_12_x86_64.whl
2022-02-14T19:31:22.4308762Z �[33mWARNING: Skipping page https://s3.amazonaws.com/ossci-linux/wheels/coverage-4.5.1-cp36-cp36m-macosx_10_12_x86_64.whl because the HEAD request got Content-Type: binary/octet-stream.The only supported Content-Type is text/html�[0m
2022-02-14T19:31:23.4235375Z + pip -q install hypothesis==3.44.6 -f https://s3.amazonaws.com/ossci-linux/wheels/hypothesis-3.44.6-py3-none-any.whl
2022-02-14T19:31:23.7636901Z �[33mWARNING: Skipping page https://s3.amazonaws.com/ossci-linux/wheels/hypothesis-3.44.6-py3-none-any.whl because the HEAD request got Content-Type: binary/octet-stream.The only supported Content-Type is text/html�[0m
2022-02-14T19:31:25.0411015Z + EXTRA_TESTS=()
2022-02-14T19:31:25.0411693Z + [[ linux-xenial-py3.7-clang7-onnx == *-cuda* ]]
2022-02-14T19:31:25.0412254Z + [[ linux-xenial-py3.7-clang7-onnx == *-rocm* ]]
2022-02-14T19:31:25.0412616Z + rocm_ignore_test=()
2022-02-14T19:31:25.0413090Z + [[ linux-xenial-py3.7-clang7-onnx == *-rocm* ]]

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

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

pytorch-bot · 2022-02-07T22:05:15Z

CI Flow Status

⚛️ CI Flow

Ruleset - Version: v1
Ruleset - File: https://github.com/pytorch/pytorch/blob/01747bfadfe9d4c9c4a1ff949561bdc145f633bf/.github/generated-ciflow-ruleset.json
PR ciflow labels: ciflow/default
Add ciflow labels to this PR to trigger more builds:

Workflows	Labels (bold enabled)	Status
Triggered Workflows
linux-binary-conda	`ciflow/binaries`, `ciflow/binaries_conda`, `ciflow/default`	✅ triggered
linux-binary-libtorch-cxx11-abi	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
linux-binary-libtorch-pre-cxx11	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
linux-binary-manywheel	`ciflow/binaries`, `ciflow/binaries_wheel`, `ciflow/default`	✅ triggered
linux-bionic-py3.7-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/noarch`, `ciflow/trunk`, `ciflow/xla`	✅ triggered
linux-bionic-rocm4.5-py3.7	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/rocm`, `ciflow/trunk`	✅ triggered
linux-docs	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/docs`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-vulkan-bionic-py3.7-clang9	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`, `ciflow/vulkan`	✅ triggered
linux-xenial-cuda11.3-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-cuda11.3-py3.7-gcc7-bazel-test	`ciflow/all`, `ciflow/bazel`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-py3-clang5-mobile-build	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/mobile`, `ciflow/trunk`	✅ triggered
linux-xenial-py3-clang5-mobile-custom-build-static	`ciflow/all`, `ciflow/default`, `ciflow/linux`, `ciflow/mobile`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-clang7-asan	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/sanitizers`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-clang7-onnx	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/onnx`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-gcc7	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
linux-xenial-py3.7-gcc7-no-ops	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-gradle-custom-build-single-full-jit	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/default`, `ciflow/linux`, `ciflow/trunk`	✅ triggered
win-vs2019-cpu-py3	`ciflow/all`, `ciflow/cpu`, `ciflow/default`, `ciflow/trunk`, `ciflow/win`	✅ triggered
win-vs2019-cuda11.3-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/default`, `ciflow/trunk`, `ciflow/win`	✅ triggered
windows-binary-libtorch-cxx11-abi	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
windows-binary-libtorch-pre-cxx11	`ciflow/binaries`, `ciflow/binaries_libtorch`, `ciflow/default`	✅ triggered
windows-binary-wheel	`ciflow/binaries`, `ciflow/binaries_wheel`, `ciflow/default`	✅ triggered
Skipped Workflows
caffe2-linux-xenial-py3.7-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
docker-builds	`ciflow/all`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-arm64	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-arm64-coreml	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-arm64-custom-ops	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-arm64-full-jit	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-arm64-metal	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-x86-64	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-x86-64-coreml	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
ios-12-5-1-x86-64-full-jit	`ciflow/all`, `ciflow/ios`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
libtorch-linux-xenial-cuda10.2-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
libtorch-linux-xenial-cuda11.3-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
linux-bionic-cuda10.2-py3.9-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/slow`, `ciflow/trunk`	🚫 skipped
linux-docs-push	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
linux-xenial-cuda11.3-py3.7-gcc7-no-ops	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
macos-10-15-py3-arm64	`ciflow/all`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
macos-10-15-py3-lite-interpreter-x86-64	`ciflow/all`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
macos-11-py3-x86-64	`ciflow/all`, `ciflow/macos`, `ciflow/trunk`	🚫 skipped
parallelnative-linux-xenial-py3.7-gcc5.4	`ciflow/all`, `ciflow/cpu`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped
periodic-libtorch-linux-bionic-cuda11.5-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-libtorch-linux-xenial-cuda11.1-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/libtorch`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-linux-bionic-cuda11.5-py3.7-gcc7	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-linux-xenial-cuda10.2-py3-gcc7-slow-gradcheck	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`, `ciflow/slow`, `ciflow/slow-gradcheck`	🚫 skipped
periodic-linux-xenial-cuda11.1-py3.7-gcc7-debug	`ciflow/all`, `ciflow/cuda`, `ciflow/linux`, `ciflow/scheduled`	🚫 skipped
periodic-win-vs2019-cuda11.1-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/scheduled`, `ciflow/win`	🚫 skipped
periodic-win-vs2019-cuda11.5-py3	`ciflow/all`, `ciflow/cuda`, `ciflow/scheduled`, `ciflow/win`	🚫 skipped
pytorch-linux-xenial-py3-clang5-android-ndk-r19c-build	`ciflow/all`, `ciflow/android`, `ciflow/cpu`, `ciflow/linux`, `ciflow/trunk`	🚫 skipped

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) [ghstack-poisoned]

Pull Request resolved: #72470 Use markdown to write an upgrader guidance ghstack-source-id: 148565616 Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/)

torch/csrc/jit/operator_upgraders/README.md

raziel

I think this needs a bit more refinement.

Try to read it as if you knew nothing... or better as if it was code and by following every step you would actually end up with a proper upgrader PR.

torch/csrc/jit/operator_upgraders/README.md

raziel · 2022-02-08T17:21:01Z

torch/csrc/jit/operator_upgraders/README.md

@@ -0,0 +1,95 @@
+# Guidance for Operator Developer


We should have an intro on what the base concepts are: operator upgrader, BC and BC breaking, ...
Keep it short and simple.
You may already have something in the design review.

raziel · 2022-02-08T17:24:45Z

torch/csrc/jit/operator_upgraders/README.md

+
+2. Make changes to the operator and write an upgrader.
+    1. Make the operator change.
+    2. Write an upgrader in `caffe2/torch/csrc/jit/operator_upgraders/upgraders_entry.cpp` file inside a map `kUpgradersEntryMap`. The softly enforced naming format is `&lt;operator_name>_&lt;operator_overload>_&lt;start>_&lt;end>`. For example, the below example means that `linspace.out `and` linspace` at versions from 0 to 7 need to be replaced by these upgraders.


will this render ok?

raziel · 2022-02-08T17:25:58Z

torch/csrc/jit/operator_upgraders/README.md

+The steps to write upgrader:
+
+1. Prepare a model before making changes to the operator, following the process:
+     1. add a module in `test/jit/fixtures_srcs/fixtures_src.py`. The reason why switching to commit is that, an old model with the old operator before the change is needed to ensure the upgrader is working as expected. In `test/jit/fixtures_srcs/generate_models.py`, add the module and its corresponding changed operator like following


Suggested change

1. add a module in `test/jit/fixtures_srcs/fixtures_src.py`. The reason why switching to commit is that, an old model with the old operator before the change is needed to ensure the upgrader is working as expected. In `test/jit/fixtures_srcs/generate_models.py`, add the module and its corresponding changed operator like following

1. Add a module in `test/jit/fixtures_srcs/fixtures_src.py`. The reason why switching to commit is that an old model with the old operator before the change is needed to ensure the upgrader is working as expected. In `test/jit/fixtures_srcs/generate_models.py`, add the module and its corresponding changed operator like following

raziel · 2022-02-08T17:26:14Z

torch/csrc/jit/operator_upgraders/README.md

+The steps to write upgrader:
+
+1. Prepare a model before making changes to the operator, following the process:
+     1. add a module in `test/jit/fixtures_srcs/fixtures_src.py`. The reason why switching to commit is that, an old model with the old operator before the change is needed to ensure the upgrader is working as expected. In `test/jit/fixtures_srcs/generate_models.py`, add the module and its corresponding changed operator like following


what does this mean?

Add more explanation and hope it's clearer.

raziel · 2022-02-08T17:27:46Z

torch/csrc/jit/operator_upgraders/README.md

+        python test/jit/fixtures_src/generate_models.py
+        ```
+
+       3. Commit the change and submit a pr


Suggested change

3. Commit the change and submit a pr

3. Commit the change and submit a pull request.

raziel · 2022-02-08T17:28:44Z

torch/csrc/jit/operator_upgraders/README.md

+
+The steps to write upgrader:
+
+1. Prepare a model before making changes to the operator, following the process:


Here you mean a test model, right?
Otherwise it's not clear what this model is mean to do or why. Please briefly explain.

Add the reason why a test model is needed.

raziel · 2022-02-08T17:32:19Z

torch/csrc/jit/operator_upgraders/README.md

+        "aten::linspace(Scalar start, Scalar end, int? steps=None, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor"}}},
+    ```
+
+    4. After rebuild PyTorch, run the following command and it will auto generate a change to `caffe2/torch/csrc/jit/mobile/upgrader_mobile.cpp`. After rebuild PyTorch from source (`python setup.py`), run


Suggested change

4. After rebuild PyTorch, run the following command and it will auto generate a change to `caffe2/torch/csrc/jit/mobile/upgrader_mobile.cpp`. After rebuild PyTorch from source (`python setup.py`), run

4. After rebuilding PyTorch, run the following command to auto generate a change to `caffe2/torch/csrc/jit/mobile/upgrader_mobile.cpp`. After rebuild PyTorch from source (`python setup.py`), run

This needs to be clearer: "auto generate a change" <--- a change that does what

We should have a step before that explicitly indicates and describes how to rebuild pytorch (or links to a page that describes that).

Also, this reads as if you have to rebuild pytorch twice.

"auto generate a change to upgrader_mobile.cpp" -> "auto update the file upgrader_mobile.cpp"

Add the link to build pytorch

Add the first time to build in step 1

raziel · 2022-02-08T17:32:58Z

torch/csrc/jit/operator_upgraders/README.md

+    python pytorch/tools/codegen/operator_versions/gen_mobile_upgraders.py
+    ```
+
+    5. Add test:


Is this a test or tests?

It was tests..now it's a test

raziel · 2022-02-08T17:36:51Z

torch/csrc/jit/operator_upgraders/README.md

+---
+**NOTE**
+
+Adding default arguments is not BC breaking and it doesn't requirement. For example, the following change is not BC breaking, and doesn't require upgrader:


Suggested change

Adding default arguments is not BC breaking and it doesn't requirement. For example, the following change is not BC breaking, and doesn't require upgrader:

Adding arguments with a default value to an operator is not BC breaking, and thus does not require upgrader. For example, the following change to operator `foo` is backwards compatible:

tugsbayasgalan · 2022-02-08T19:46:56Z

torch/csrc/jit/operator_upgraders/README.md

+
+2. Make changes to the operator and write an upgrader.
+    1. Make the operator change.
+    2. Write an upgrader in `caffe2/torch/csrc/jit/operator_upgraders/upgraders_entry.cpp` file inside a map `kUpgradersEntryMap`. The softly enforced naming format is `&lt;operator_name>_&lt;operator_overload>_&lt;start>_&lt;end>`. For example, the below example means that `linspace.out `and` linspace` at versions from 0 to 7 need to be replaced by these upgraders.


Maybe we should explain what version is? Because for general public, it is not very clear

Add more explanation.

tugsbayasgalan · 2022-02-08T19:47:23Z

torch/csrc/jit/operator_upgraders/README.md

+        }
+    ```
+
+    2. Bump the `kProducedFileFormatVersion` by 1 and provide the reasons under `caffe2/caffe2/versions.h `


I think you also need to bump kMaxProducedFileFormatVersion

tugsbayasgalan · 2022-02-08T19:48:38Z

torch/csrc/jit/operator_upgraders/README.md

+    constexpr uint64_t kProducedFileFormatVersion = 0x9L;
+    ```
+
+    3. In `caffe2/torch/csrc/jit/operator_upgraders/version_map.cpp`, add changes like below. You will need to make sure that the entry is **SORTED** according to the version bump number.


Maybe mention how the below example is formatted according to what we describe here

Added an example to explain

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) [ghstack-poisoned]

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Pull Request resolved: #72470 Use markdown to write an upgrader guidance ghstack-source-id: 148776976 Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/)

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Pull Request resolved: #72470 Use markdown to write an upgrader guidance ghstack-source-id: 148895783 Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/)

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Pull Request resolved: #72470 Use markdown to write an upgrader guidance ghstack-source-id: 149033187 Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/)

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Pull Request resolved: #72470 Use markdown to write an upgrader guidance ghstack-source-id: 149066628 Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/)

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Pull Request resolved: #72470 Use markdown to write an upgrader guidance ghstack-source-id: 149079223 Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/)

Summary: Pull Request resolved: #72470 Use markdown to write an upgrader guidance ghstack-source-id: 149079223 Test Plan: CI Reviewed By: iseeyuan, tugsbayasgalan Differential Revision: D34054964 fbshipit-source-id: 80f907701a9d44bcd6b5c22d01a471c26669bfb0

github-actions · 2022-02-14T20:33:03Z

Hey @cccclai.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

raziel

I just reviewed the top since this is a closed PR and cannot make suggestions in the content itself better for me to continue my review once you start a new PR.

raziel · 2022-02-14T22:21:12Z

torch/csrc/jit/operator_upgraders/README.md

@@ -0,0 +1,205 @@
+# Guidance for Operator Developer
+
+PyTorch’s operators sometimes require changes to maintain the high quality user experience (UX) that PyTorch is known for. These changes can be backward compatibility (BC) breaking, where older programs will no longer run as expected on the latest version of PyTorch (an old writer / new reader problem) or forward compatibility (FC) breaking, where new programs will not run on older versions of PyTorch (a new writer / old reader problem). An upgrader is a method to use the new operator to mimic the old operator behavior. When a new runtime loads an old model with the old operator, the upgrader will replace the old operator in the model with the new operator. The replacement will only happen for old models, and it does not need to consider the new models. Please refer to the documentation [PyTorch Operator Versioning](https://github.com/pytorch/rfcs/blob/master/RFC-0017-PyTorch-Operator-Versioning.md) for more details.


A few things:

I would rephrase old write / new reader to actually talk about program and runtime.

We talk about FC but we should clarify upgraders are just for BC>

"The replacement will only happen for old models, and it does not need to consider the new models." => Shouldn't this just be as simple as "upgraders only apply to instances of old operators to make sure they comply with the new operator contract"?

Address on the new pr

raziel · 2022-02-14T22:28:37Z

torch/csrc/jit/operator_upgraders/README.md

+
+PyTorch’s operators sometimes require changes to maintain the high quality user experience (UX) that PyTorch is known for. These changes can be backward compatibility (BC) breaking, where older programs will no longer run as expected on the latest version of PyTorch (an old writer / new reader problem) or forward compatibility (FC) breaking, where new programs will not run on older versions of PyTorch (a new writer / old reader problem). An upgrader is a method to use the new operator to mimic the old operator behavior. When a new runtime loads an old model with the old operator, the upgrader will replace the old operator in the model with the new operator. The replacement will only happen for old models, and it does not need to consider the new models. Please refer to the documentation [PyTorch Operator Versioning](https://github.com/pytorch/rfcs/blob/master/RFC-0017-PyTorch-Operator-Versioning.md) for more details.
+
+After you change to operator either the operator schema is BC-breaking way or the semantics of the operator, you will need to write an “upgrader” to make the change non-BC breaking iff they are used in TorchScript or mobile. In general, you can know your operator is BC breaking, if it fails `test/forward_backward_compatibility/check_forward_backward_compatibility.py `


There's typos here: "After you change to operator either the operator schema is BC-breaking way or the semantics of the operator"

Also, this should at least include some examples on what's BC breaking.

"change non-BC breaking iff they are used in TorchScript or mobile"
Just for mobile? Also how could anybody know that: they can't so this is not optional.

Address on the new pr

cccclai · 2022-02-14T23:28:59Z

@raziel thanks for the feedback. I start a new pr #72818 to address the comments.

Summary: Pull Request resolved: pytorch/pytorch#72470 Use markdown to write an upgrader guidance ghstack-source-id: 149079223 Test Plan: CI Reviewed By: iseeyuan, tugsbayasgalan Differential Revision: D34054964 fbshipit-source-id: 80f907701a9d44bcd6b5c22d01a471c26669bfb0 (cherry picked from commit eaed7627f526cab932273d24002a4f8e4a92c824)

Operator developer guidance

01747bf

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) [ghstack-poisoned]

pytorch-bot bot added the ciflow/default label Feb 7, 2022

facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Feb 7, 2022

cccclai requested review from gmagogsfm, iseeyuan, raziel and tugsbayasgalan February 7, 2022 22:11

Update on "Operator developer guidance"

3d20eda

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) [ghstack-poisoned]

Update on "Operator developer guidance"

30ae66b

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) [ghstack-poisoned]

gmagogsfm reviewed Feb 8, 2022

View reviewed changes

torch/csrc/jit/operator_upgraders/README.md Show resolved Hide resolved

raziel reviewed Feb 8, 2022

View reviewed changes

tugsbayasgalan reviewed Feb 8, 2022

View reviewed changes

Update on "Operator developer guidance"

6cbf6b3

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) [ghstack-poisoned]

cccclai mentioned this pull request Feb 9, 2022

Ensure all modules in the dict are instance instead of class #72615

Closed

Update on "Operator developer guidance"

2adcdf9

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Update on "Operator developer guidance"

6e26fdf

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Update on "Operator developer guidance"

e53bb4f

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

cccclai marked this pull request as draft February 10, 2022 03:27

Update on "Operator developer guidance"

87e2296

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Update on "Operator developer guidance"

0863405

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Update on "Operator developer guidance"

bc4d7ae

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

Update on "Operator developer guidance"

cccc60d

Use markdown to write an upgrader guidance Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964/) Differential Revision: [D34054964](https://our.internmc.facebook.com/intern/diff/D34054964) [ghstack-poisoned]

pytorchmergebot closed this in 8e8c15c Feb 14, 2022

raziel reviewed Feb 14, 2022

View reviewed changes

facebook-github-bot deleted the gh/cccclai/159/head branch February 18, 2022 15:17

	1. add a module in `test/jit/fixtures_srcs/fixtures_src.py`. The reason why switching to commit is that, an old model with the old operator before the change is needed to ensure the upgrader is working as expected. In `test/jit/fixtures_srcs/generate_models.py`, add the module and its corresponding changed operator like following
	1. Add a module in `test/jit/fixtures_srcs/fixtures_src.py`. The reason why switching to commit is that an old model with the old operator before the change is needed to ensure the upgrader is working as expected. In `test/jit/fixtures_srcs/generate_models.py`, add the module and its corresponding changed operator like following

	3. Commit the change and submit a pr
	3. Commit the change and submit a pull request.


		The steps to write upgrader:

		1. Prepare a model before making changes to the operator, following the process:

	4. After rebuild PyTorch, run the following command and it will auto generate a change to `caffe2/torch/csrc/jit/mobile/upgrader_mobile.cpp`. After rebuild PyTorch from source (`python setup.py`), run
	4. After rebuilding PyTorch, run the following command to auto generate a change to `caffe2/torch/csrc/jit/mobile/upgrader_mobile.cpp`. After rebuild PyTorch from source (`python setup.py`), run

	Adding default arguments is not BC breaking and it doesn't requirement. For example, the following change is not BC breaking, and doesn't require upgrader:
	Adding arguments with a default value to an operator is not BC breaking, and thus does not require upgrader. For example, the following change to operator `foo` is backwards compatible:

		@@ -0,0 +1,205 @@
		# Guidance for Operator Developer

		PyTorch’s operators sometimes require changes to maintain the high quality user experience (UX) that PyTorch is known for. These changes can be backward compatibility (BC) breaking, where older programs will no longer run as expected on the latest version of PyTorch (an old writer / new reader problem) or forward compatibility (FC) breaking, where new programs will not run on older versions of PyTorch (a new writer / old reader problem). An upgrader is a method to use the new operator to mimic the old operator behavior. When a new runtime loads an old model with the old operator, the upgrader will replace the old operator in the model with the new operator. The replacement will only happen for old models, and it does not need to consider the new models. Please refer to the documentation [PyTorch Operator Versioning](https://github.com/pytorch/rfcs/blob/master/RFC-0017-PyTorch-Operator-Versioning.md) for more details.


		PyTorch’s operators sometimes require changes to maintain the high quality user experience (UX) that PyTorch is known for. These changes can be backward compatibility (BC) breaking, where older programs will no longer run as expected on the latest version of PyTorch (an old writer / new reader problem) or forward compatibility (FC) breaking, where new programs will not run on older versions of PyTorch (a new writer / old reader problem). An upgrader is a method to use the new operator to mimic the old operator behavior. When a new runtime loads an old model with the old operator, the upgrader will replace the old operator in the model with the new operator. The replacement will only happen for old models, and it does not need to consider the new models. Please refer to the documentation [PyTorch Operator Versioning](https://github.com/pytorch/rfcs/blob/master/RFC-0017-PyTorch-Operator-Versioning.md) for more details.

		After you change to operator either the operator schema is BC-breaking way or the semantics of the operator, you will need to write an “upgrader” to make the change non-BC breaking iff they are used in TorchScript or mobile. In general, you can know your operator is BC breaking, if it fails `test/forward_backward_compatibility/check_forward_backward_compatibility.py `

Operator developer guidance #72470

Operator developer guidance #72470

Uh oh!

Conversation

cccclai commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

linux-xenial-py3.7-clang7-onnx / test (default, 1, 2, linux.2xlarge) (1/1)

Uh oh!

pytorch-bot bot commented Feb 7, 2022

⚛️ CI Flow

Uh oh!

Uh oh!

raziel left a comment

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!

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!

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!

github-actions bot commented Feb 14, 2022

Uh oh!

raziel 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!

cccclai commented Feb 14, 2022

Uh oh!

Reviewers

cccclai commented Feb 7, 2022 •

edited

Loading

facebook-github-bot commented Feb 7, 2022 •

edited

Loading