Summary:
`ObjLoaderFunc` calls `restoreAccurateTypeTags` on every pickled object that
has a `__setstate__`, unconditionally. For modern archives that call finds
nothing to correct, and it is extremely expensive.
Per [type tag serialization] (`unpickler.h`), the two tag-recovery paths are
mutually exclusive:
- **version <= 2**: recover tags by recursively descending the object graph.
- **version >= 3**: the archive carries container type strings, and the
unpickler applies them as each container is constructed
(`restore_type_tag` -> `restoreContainerTypeTags`, `unpickler.cpp`).
`Unpickler::parse_ivalue` already gates its own call on `version_ <= 2`.
`ObjLoaderFunc` never got the same gate, most likely because its signature had
no way to see the archive version.
## Shape of the change
`ObjLoaderFunc` keeps its **exact existing signature**, so it still converts to
`ObjLoader` and every current caller is untouched. A new
`ObjLoaderFuncWithVersion` takes the archive version and applies the gate;
`ObjLoaderFunc` delegates to it with 0 ("unknown"), which is `<= 2` and so
preserves today's behaviour. Only `ScriptModuleDeserializer::readArchive`
opts in, passing `reader_->version()`.
An earlier revision of this diff instead added a defaulted third parameter to
`ObjLoaderFunc`. That is subtly wrong: default arguments are not part of a
function's type, so the two-argument `ObjLoader` conversion stops working. It
broke `caffe2/torch/fb/distributed/wireSerializer` (`WireSerializer.cpp:366`),
which was caught by a ServiceLab build. The current shape avoids the problem by
not touching the existing signature at all.
## Why it matters
Measured on the CG1 Sigrid predictor (tenant m2125804315), Cogwheel workload
`cogwheel_ipnext_cw_inplace_stream_m2125804315_slatest_default_cg1`,
ServiceLab E1727110318567547:
```
[RESTORE_TAGS] big_call works=1280000009 ptr_works=320000004 scanned=320000004 ms=45442
root_type=Tuple[Tuple[int, float, float, List[Tuple[int, int, int]]], Optional[int], Tensor]
[RESTORE_TAGS] big_call kinds: TensorType:1 TupleType:320000002 ListType:1 FloatType:2 IntType:960000002 OptionalType:1
[RESTORE_TAGS] big_call tags list_ptr_eq=0 list_struct_eq=1 list_differs=0 dict_ptr_eq=0 dict_struct_eq=0 dict_differs=0
```
One `List[Tuple[int, int, int]]` holding 320M tuples. The traversal processes
**1.28 billion** work items, at **45 s per model load**, three loads per test.
Its entire useful output is setting the element type on that one list, to the
value it already had.
The decisive number is `list_differs=0` / `dict_differs=0`, holding on every
call, cumulatively across all 16 calls, on both sides of an A/A run: **no
container needed retagging**. The v>=3 path had already done the work.
## Safety
- The condition is identical to the one `unpickler.cpp` has used for years.
- Every existing caller keeps the old behaviour; only the ScriptModule load
path opts in.
- `logTypeTagPolicyOnce` emits one INFO line recording the archive version and
which path was taken, so the gate is observable in a deployed binary rather
than having to be inferred from timing.
- If the gate were ever wrong it fails loudly and immediately: a container
arrives as `List[Any]` and `__setstate__`'s type check rejects it at load
time, rather than silently mistagging.
Related: D115295360 changed the container inside this function and made it
2.73x slower (115 s vs 42 s per load); D116346069 reverts that. This diff
removes the call for modern archives, which subsumes the problem on that path.
Test Plan:
## Tests
Three cases in `caffe2/test/cpp/jit/test_save_load.cpp`, next to the existing
`SerializationTest.TypeTags`. Added in response to review: the version <= 2
branch had no coverage.
- `ObjLoaderRestoresTypeTagsForLegacyArchive` -- feeds the loader the shape an
unpickled version <= 2 archive produces (right values, `List[Any]` tags) and
asserts the tags are corrected in place, on the nested container as well as
the outer one. This is the branch the review asked about.
- `ObjLoaderWithoutVersionRestoresTypeTags` -- the two-argument `ObjLoaderFunc`
has no version to go on and must stay on the legacy path. This pins the
compatibility promise that existing `ObjLoader` callers depend on.
- `ObjLoaderSkipsTypeTagRestorationForModernArchive` -- an already-tagged state
(what a version >= 3 archive produces) loads untouched; a generically tagged
one is passed through unchanged, so `__setstate__`'s own type check rejects
it. That second half is what makes the skip observable, and documents that
the path is only safe because the unpickler tags containers eagerly.
`test_save_load.cpp` is already listed in both `BUCK` and `CMakeLists.txt`, so
no build-file change was needed and OSS CI picks the cases up.
## How they were verified
**The shared gtest binary could not run them.**
`//caffe2/test/cpp/jit:jit` segfaults during static initialization, before any
test body executes. This is pre-existing and unrelated to this diff -- it
reproduces on an unmodified tree, and a filter matching *zero* tests segfaults
identically:
```
$ ./jit --gtest_filter=ThisMatchesNothingAtAll ; echo $?
Segmentation fault
139
$ ./jit --gtest_list_tests ; echo $?
Segmentation fault
139
```
So the assertions were validated by a standalone binary linking
`//caffe2:torch-cpp` and running the same three scenarios:
```
[version 2: legacy archive, tags restored]
PASS outer element type got=List[int] want=List[int]
PASS inner element type got=int want=int
[no version: two-arg overload keeps legacy behaviour]
PASS outer element type got=List[int] want=List[int]
[version 3: modern archive, traversal skipped]
PASS already-tagged state loads without throwing
PASS tagged element type unchanged got=List[int] want=List[int]
PASS generically tagged state is rejected by __setstate__
PASS untagged element type left alone got=Any want=Any
ALL CHECKS PASSED
```
**Mutation check.** To confirm the legacy case actually exercises the gate
rather than passing trivially, the gate was temporarily broken
(`archive_version <= 2` -> `<= 1`, so version 2 takes the skip path) and the
same binary re-run. It fails, in exactly the way the production code would:
```
__setstate__() Expected a value of type 'List[List[int]]' for argument 'state'
but instead found type 'List[Any]'.
Declaration: __setstate__(__torch__.m self, int[][] state) -> NoneType
# 6 torch::jit::ObjLoaderFuncWithVersion(...)
```
The mutation was reverted and the binary re-run clean before submitting.
To be explicit about the gap: the cases are correct and their logic is
verified, but I have not been able to watch them execute under gtest itself
because of the pre-existing crash in that target. OSS CI will be the first
gtest run.
## Build
Clean, exit code captured directly rather than through a pipe. Includes
`wire_serializer`, which an earlier revision of this diff broke:
```
$ buck2 build fbcode//mode/opt fbcode//caffe2:_libtorch \
fbcode//caffe2/torch/fb/distributed/wireSerializer:wire_serializer > /tmp/b.log 2>&1; echo "REAL_EXIT=$?"
REAL_EXIT=0
```
`arc lint -e extra` reports nothing on the files this diff touches. The two
remaining CLANGTIDY findings (`bugprone-unchecked-optional-access`,
`clang-diagnostic-range-loop-bind-reference`) are pre-existing in
`recreateObject` and are not touched here.
## Cogwheel A/B vs current prod
ServiceLab **E2105961143339521**, workload
`cogwheel_ipnext_cw_inplace_stream_m2125804315_slatest_default_cg1`,
side A = current prod, side B = this diff.
```
[PASSED] [Test Suite] (1417.27s)
[PASSED] test_predictor_ab_response_checker (732.69s)
```
Gate fired, and only on side B, which also proves the candidate binary bound:
```
side b: I0818 18:31:47 import.cpp:77] [jit] restoreAccurateTypeTags: archive_version=4 -> skipped (see [type tag serialization])
side a: (no such line - prod does not carry this change)
```
Startup, measured from `parseSubmodToUVMString` to `Setting fb303 status to ALIVE`:
| side | marker | ALIVE | elapsed |
|---|---|---|---|
| A (current prod) | 18:31:22.220 | 18:35:00.502 | 218.3 s |
| B (this diff) | 18:31:22.199 | 18:33:11.366 | **109.2 s** |
**109.1 s faster per cold start.** Response correctness is unchanged --
`test_predictor_ab_response_checker` compares side B's outputs against prod and
passed, which is the check that would catch a container left mistagged.
For reference, the same measurement from E2245630172674916, which A/B'd the two
containers with identical instrumentation:
| config | parseSubmod -> ALIVE |
|---|---|
| `std::unordered_set` (what D116346069 restores) | 147.4 s |
| `c10::FastSet` (master) | 218.6 s |
| current prod | 218.3 s |
| **this diff** | **109.2 s** |
Two things follow. Current prod's startup profile matches the `c10::FastSet`
arm rather than the `std::unordered_set` arm, so prod is already carrying the
D115295360 regression. And skipping the call outright is ~38 s better than
reverting the container, because it removes the whole traversal rather than
making it cheaper.
Differential Revision: D116540489
Pull Request resolved: https://github.com/pytorch/pytorch/pull/194035
Approved by: https://github.com/frgossen
Fixes#75177
Couldn't find any utility method to get directory name in pytorch repo, hence creating a function for that.
Let me know if a new function is not needed.
I also referred [this](https://github.com/pytorch/pytorch/blob/master/c10/test/util/tempfile_test.cpp#L15) for directory check.
Also I am using TORCH_CHECK to show the error. This is highly verbose with the entire stack visible. Is there any alternative for the same so that it is easier to read? This could happen a frequently, so small and concise error would be more helpful here.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/75681
Approved by: https://github.com/albanD
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/74119
implemented function to generate source as ExtraFilesMap and constants
wrote function to construct jit module given (ivalue, source,
constant) tripple.
Test Plan: unittest
Reviewed By: pavithranrao
Differential Revision: D34803945
fbshipit-source-id: 2edc798407fe68294cb4c3c7516f5bd143df88c3
(cherry picked from commit 35e54e166b)
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/64066
I noticed a bunch of time being spent heap-allocating Tuples
in the unpickler. 1-, 2-, and 3-element Tuples are apparently common
enough that they get their own bytecode instructions, so I decided to
try also giving them their own representation. We store up to 3
IValues inline in `Tuple` rather than doing a second heap allocation
for a `std::vector<IValue>`.
ghstack-source-id: 140695395
Test Plan:
Added automated tests for TupleElements.
Pixel 3 before: https://www.internalfb.com/intern/aibench/details/761596366576284
Pixel 3 after: https://www.internalfb.com/intern/aibench/details/591414145082422
We went from 347 ms to 302 ms.
Reviewed By: dhruvbird
Differential Revision: D30592622
fbshipit-source-id: 93625c54c9dca5f765ef6d5c191944179cb281a8
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/65345
FooType::get() can return a const reference. Inconveniently, converting shared_ptr<FooType> to shared_ptr<Type> requires a copy & refcount bump, so to properly take advantage of this in unshapedType() we need to take a const Type& in isSubtypeOf(), which is good practice anyway -- don't require a shared_ptr if you don't need to take ownership.
ghstack-source-id: 140044165
Test Plan:
CI
perf says c10::unshapedType time decreased from 2.8% to 2.2% during static runtime startup, though I expect this to be generally beneficial.
Reviewed By: hlu1
Differential Revision: D31027361
fbshipit-source-id: 676feb81db9f74ad7b8651d8774f4ecb4cfa6ab8
Summary:
As GoogleTest `TEST` macro is non-compliant with it as well as `DEFINE_DISPATCH`
All changes but the ones to `.clang-tidy` are generated using following script:
```
for i in `find . -type f -iname "*.c*" -or -iname "*.h"|xargs grep cppcoreguidelines-avoid-non-const-global-variables|cut -f1 -d:|sort|uniq`; do sed -i "/\/\/ NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)/d" $i; done
```
Pull Request resolved: https://github.com/pytorch/pytorch/pull/62008
Reviewed By: driazati, r-barnes
Differential Revision: D29838584
Pulled By: malfet
fbshipit-source-id: 1b2f8602c945bd4ce50a9bfdd204755556e31d13
Summary:
This is an automatic change generated by the following script:
```
#!/usr/bin/env python3
from subprocess import check_output, check_call
import os
def get_compiled_files_list():
import json
with open("build/compile_commands.json") as f:
data = json.load(f)
files = [os.path.relpath(node['file']) for node in data]
for idx, fname in enumerate(files):
if fname.startswith('build/') and fname.endswith('.DEFAULT.cpp'):
files[idx] = fname[len('build/'):-len('.DEFAULT.cpp')]
return files
def run_clang_tidy(fname):
check_call(["python3", "tools/clang_tidy.py", "-c", "build", "-x", fname,"-s"])
changes = check_output(["git", "ls-files", "-m"])
if len(changes) == 0:
return
check_call(["git", "commit","--all", "-m", f"NOLINT stubs for {fname}"])
def main():
git_files = check_output(["git", "ls-files"]).decode("ascii").split("\n")
compiled_files = get_compiled_files_list()
for idx, fname in enumerate(git_files):
if fname not in compiled_files:
continue
if fname.startswith("caffe2/contrib/aten/"):
continue
print(f"[{idx}/{len(git_files)}] Processing {fname}")
run_clang_tidy(fname)
if __name__ == "__main__":
main()
```
Pull Request resolved: https://github.com/pytorch/pytorch/pull/56892
Reviewed By: H-Huang
Differential Revision: D27991944
Pulled By: malfet
fbshipit-source-id: 5415e1eb2c1b34319a4f03024bfaa087007d7179
Summary:
=======
This PR addresses the following:
* Adds JIT support for CUDA Streams
* Adds JIT support for CUDA Events
* Adds JIT support for CUDA Stream context manager
Testing:
======
python test/test_jit.py -v TestCUDA
Pull Request resolved: https://github.com/pytorch/pytorch/pull/48020
Reviewed By: navahgar
Differential Revision: D25725749
Pulled By: nikithamalgifb
fbshipit-source-id: b0addeb49630f8f0c430ed7badeca43bb9d2535c
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/45264
Context for why we are porting to gtest in: https://github.com/pytorch/pytorch/pull/45018.
This PR completes the process of porting and removes unused files/macros.
Test Plan: Imported from OSS
Reviewed By: ZolotukhinM
Differential Revision: D23901392
Pulled By: suo
fbshipit-source-id: 89526890e1a49462f3f77718f4ee273c5bc578ba
Summary:
BC NOTE:
This change makes it so modules saved with torch.jit.save in PyTorch 1.6 can be loaded by previous versions of PyTorch unless they use torch.div or (soon) torch.full. It also lets tensors saved using torch.save be loaded by previous versions. So this is the opposite of BC-breaking, but I'm using that label to highlight this issue since we don't have a "BC-improving" label.
PR NOTE:
When an operator's semantics change in PyTorch we want to do two things:
1) Preserve the semantics of older serialized Torchscript programs that use the operator
2) Ensure the new semantics are respected
Historically, this meant writing a Versioned Symbol that would remap older versions of the operator into current PyTorch code (1), and bumping the produced file format version (2). Unfortunately, bumping the produced file format version is a nuclear option for ensuring semantics are respected, since it also prevents older versions of PyTorch from loading anything (even tensors!) from newer versions.
Dynamic versioning addresses the nuclear consequences of bumping the produced file format version by only bumping it when necessary. That is, when an operator with changed semantics is detected in the serialized Torchscript. This will prevent Torchscript programs that use the changed operator from loading on earlier versions of PyTorch, as desired, but will have no impact on programs that don't use the changed operator.
Note that this change is only applicable when using torch.jit.save and torch.jit.load. torch.save pickles the given object using pickle (by default), which saves a function's Python directly.
No new tests for this behavior are added since the existing tests for versioned division in test_save_load already validate that models with div are loaded correctly at version 4.
Pull Request resolved: https://github.com/pytorch/pytorch/pull/40279
Reviewed By: dzhulgakov
Differential Revision: D22168291
Pulled By: mruberry
fbshipit-source-id: e71d6380e727e25123c7eedf6d80e5d7f1fe9f95
Summary:
Just run `./tools/clang_format.py --verbose` and `git commit --all`
Pull Request resolved: https://github.com/pytorch/pytorch/pull/35969
Test Plan: CI
Differential Revision: D20845626
Pulled By: malfet
fbshipit-source-id: 0ae9a91dfa33417a021e7e9d233baba4188daf81
Summary:
This enables the serialization part of this change (the deserialization stuff is already landed #33255)
Pull Request resolved: https://github.com/pytorch/pytorch/pull/35741
Pulled By: driazati
Differential Revision: D20758124
fbshipit-source-id: e2cdefa99c3bec991491e5e967e7f1661ca7ffd9
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/35115
This commit runs the newly added tools/clang_format.py on the JIT
codebase and includes all of the formatting changes thus produced.
Testing:
Ran the script, CI.
Test Plan: Imported from OSS
Reviewed By: eellison
Differential Revision: D20568523
Pulled By: SplitInfinity
fbshipit-source-id: e09bdb982ccf090eecfb7c7b461b8d0681eef82b
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/34515
Once upon a time we thought this was necessary. In reality it is not, so
removing it.
For backcompat, our public interface (defined in `api/`) still has
typedefs to the old `script::` names.
There was only one collision: `Pass` as a `Stmt` and `Pass` as a graph
transform. I renamed one of them.
Test Plan: Imported from OSS
Differential Revision: D20353503
Pulled By: suo
fbshipit-source-id: 48bb911ce75120a8c9e0c6fb65262ef775dfba93
Summary:
Stacked PRs
* #33474 - [jit] Remove list specializations from pickler
* **#33255 - [jit] Add type tags to lists/dicts in pickle**
This adds a global call to `torch.jit._pickle.restore_type_tags` for
lists and dicts so that we can preserve their types after serialization.
](https://our.intern.facebook.com/intern/diff/20346780/)
Pull Request resolved: https://github.com/pytorch/pytorch/pull/33255
Pulled By: driazati
Differential Revision: D20346780
fbshipit-source-id: c8534954ef4adb2e3c880401acbee30cd284f3db
Summary:
Stacked PRs
* #33474 - [jit] Remove list specializations from pickler
* **#33255 - [jit] Add type tags to lists/dicts in pickle**
This adds a global call to `torch.jit._pickle.restore_type_tags` for
lists and dicts so that we can preserve their types after serialization.
](https://our.intern.facebook.com/intern/diff/19868637/)
Pull Request resolved: https://github.com/pytorch/pytorch/pull/33255
Pulled By: driazati
Reviewed By: xman1979, Tianshu-Bao
Differential Revision: D19868637
fbshipit-source-id: 2f1826e6679a786ca209198690269f399a542c04
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/28129
The previous PR in the stack removed the need to order classes/functions
or have correct import statements. This resolved circular depedency issues
that can arise when class constructors like ModuleList put new instances
of themselves in a common namespace.
This PR changes our export format to no longer produce this information.
By doing so we can make the logic signficantly simpler, since we just
keep track of an individual PythonPrint object per file.
Notes:
* PythonPrint was changed to manage its own stream/list of ranges. It
was doing this anyway internally, this just makes the API more clear.
* Since we are changing the serialization format, I also removed op_version_set.
It is now replaced with the VERSION number that written in the zip archive.
This further simplifies the code emission process.
* A test of op_version_set was removed since there is no longer any behavior
to test.
Test Plan: Imported from OSS
Differential Revision: D17961610
Pulled By: zdevito
fbshipit-source-id: ada362c4ca34d05393a1a7e799c94785ab9d9825
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/26758
This PR changes the order in which we import classes and functions so
that is is no longer necessary for them to defined in order in a file,
or for there to be proper import statements in the exported file.
Actually importing a function/class now is driven by the need to resolve
the entity during unpickling, type resolution, or value resolution.
While this should allow significant simplification to the code that
serializes classes, this work has not been done yet in order to avoid
inevitable forward compat issues in the transition period.
Notes:
* Individual functions have been replaced with a SourceImporter object
that exposes a resolveType method. This method loads the type if
it has not been loaded yet, potentially parsing (but not loading)
the file it exists in if that file hasn't been parsed yet.
* Some legacy functionality needed to be added as a method to this object
since the old format still used some of this logic for class resolution.
Test Plan: Imported from OSS
Differential Revision: D17558989
Pulled By: zdevito
fbshipit-source-id: 7eae3470bcbd388c4de463e3462d527776ed46c6
Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/24801
This is to fix the ODR-violations in fbcode static builds, which have been broken for several months.
This PR is unfortunately quite large, but the changes are only mechanical:
1. Tests defined in header files -> tests defined in cpp files
2. Remove the `torch::jit::testing` namespace -> `torch::jit`.
3. Single `test.h` file that aggregates all tests.
4. Separate out files for gtest and python versions of the tests instead of using a build flag
5. Add a readme for how to add a new test, and explaining a bit about why the cpp tests are the way they are.
Test Plan: Imported from OSS
Differential Revision: D16878605
Pulled By: suo
fbshipit-source-id: 27b5c077dadd990a5f74e25d01731f9c1f491603
Joshua Su