Conversation
💊 CI failures summary and remediationsAs of commit a1b15c6 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
|
See #31877. Will this add more complexity to this discussion? How is a user or tool going to be able to tell from an automatically allocated version number which version of PyTorch generated the file or needs to be installed to reproduce the behavior? |
You can't tell which version of PyTorch generated a particular version number, since it's not a unique mapping (even today) from PyTorch version -> produced format number. You can tell which versions support it by looking at the supported format number, just like today. |
Scenario: TorchScript How can a skilled user or a tool tell which min version of PyTorch is needed to load this file? |
I don't believe this is a supported scenario today. But I also don't think that's a very interesting scenario. Why would someone upgrade to the minimum version of PyTorch that supports the .pt file in question as opposed to just upgrading to the latest PyTorch? |
|
The general issue is how to consistently represent the format information to the user. There are tools (trying) to support PyTorch. Users report bugs and the complexities of the different PyTorch formats are hard to follow. For example, a TorchScript file saved with v4 could be called "TorchScript v4". What about a Zip container that was saved using |
That's not an issue this PR is trying to address. Although it does add some complications to the discussion. You could say, "this is a file that requires the consumer to support version X," which is effectively what our error messages say if you try to read these files on versions of PyTorch that are too old to understand them. |
zdevito
left a comment
There was a problem hiding this comment.
Looks good. One small nit about where the version logic is handled.
| explicit PyTorchStreamWriter( | ||
| const std::function<size_t(const void*, size_t)>& writer_func); | ||
| std::string archive_name, | ||
| const bool _write_version_at_setup=true); |
There was a problem hiding this comment.
Not a huge fan of a flag here. Makes the behavior complicated. I feel like the version should just get written on finalization (seems like we already have a finalized_ flag). And inline container should handle the maxing.
There was a problem hiding this comment.
Good idea. I wasn't a fan of it either but was leery of changing inline_container too much. Moving this to finalization should be nicer.
…mic_file_format
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
9da9ce6 to
6c0ad6b
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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: pytorch#40279 Reviewed By: dzhulgakov Differential Revision: D22168291 Pulled By: mruberry fbshipit-source-id: e71d6380e727e25123c7eedf6d80e5d7f1fe9f95
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:
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.