Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test to validate torchscript backward compatibility #838

Merged
merged 4 commits into from
Aug 6, 2020

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Jul 29, 2020

This PR adds CI test to check the backward compatibility of Torchscript functions/object dumps.

The job first dumps Torchscript objects into file from torchaudio 0.6.0 release environment (x Python 3.6, 3.7, 3,8), then load & run the function in master build (in Python 3.6, 3.7, 3.8).

If there is a BC-breaking change in master build, (registration schema change), then the test should fail. See an example here: https://app.circleci.com/pipelines/github/pytorch/audio/3026/workflows/84a75b5f-709d-43f7-a381-463cd96905f6/jobs/83711, which is part of #816 where sample_rate option is added to schema but the original schema was not retained.

At this moment, info function does not work due to suspected bug in torch side, so the test is disabled for info function. See pytorch/pytorch#42258 for the detail. Once pytorch/pytorch#42258 is resolved we can enable it.

Depends on #842

@mthrok mthrok force-pushed the torchscript-bc-test branch from e8e10b0 to b11b3c9 Compare August 5, 2020 21:10


```
./main.py --mode validate --version 0.6.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more appropriate in a unittest. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think so because unit tests can be just run. This test suite has a set of constraints and is more opinionated in how it should be run. Mixing them would make it harder to manage.
What is your rational on putting this in a unit test?

Copy link
Contributor

@vincentqb vincentqb Aug 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was saying this because the validation takes a checkpoint and loads it -- this feels like a unit that can be done independently. However, the main point of not doing all of this through unittest is that loading of course depends on saving. and the latter doesn't fit well with unittest. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new torchscript BC test is novel and unique to PyTorch, so I think we should separate it from something as simple as unit test. Following pytorch/pytorch#42258, we have high chance that we need to change the implementation of AudioMetaData class, and we need to correctly understand the impact and the test strategy there. So keeping it separate from unit test gives better dev x and make the project more manageable. I would say this is the form of separation of concerns for project maintanance.

@@ -0,0 +1,131 @@
# Torchaudio Unit Test Suite
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i'd make this move a separate PR :)

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a large amount of new infrastructure needed to save then load. Would there be a way of leveraging the existing unittest environment?

@mthrok
Copy link
Collaborator Author

mthrok commented Aug 6, 2020

There's a large amount of new infrastructure needed to save then load. Would there be a way of leveraging the existing unittest environment?

The CI scripts included in this PR give better abstraction of environment setup and thus the better approach is to use the new scripts in unit test CI setup, which we can do in the later PR. I even consider to put them outside of .circleci directory and maybe in tools because it's very useful.

@vincentqb
Copy link
Contributor

The CI scripts included in this PR give better abstraction of environment setup and thus the better approach is to use the new scripts in unit test CI setup, which we can do in the later PR. I even consider to put them outside of .circleci directory and maybe in tools because it's very useful.

Great, then would updating the abstractions first and then adding torchscript backward compatibility tests be safer to do?

@mthrok
Copy link
Collaborator Author

mthrok commented Aug 6, 2020

The CI scripts included in this PR give better abstraction of environment setup and thus the better approach is to use the new scripts in unit test CI setup, which we can do in the later PR. I even consider to put them outside of .circleci directory and maybe in tools because it's very useful.

Great, then would updating the abstractions first and then adding torchscript backward compatibility tests be safer to do?

I am not sure what you mean by safe. If you are talking about the breakage of CI system then the answer is no. Keeping them independent is much safer.

However what is more valuable than merging two underlying scripts is the value this backward compatibility test brings. This will make sure that PRs like #840 and #816 will not be BC-braking. Also due to pytorch/pytorch#42258 we might need to change info implementation, which has a way higher project impact/priority. So I do not think prioritizing on tool clean up is a good way to proceed.

@vincentqb
Copy link
Contributor

I am not sure what you mean by safe.

Fair enough :)

So I do not think prioritizing on tool clean up is a good way to proceed.

I agree with this.

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty, thanks for the clarification! LGTM.

@mthrok mthrok merged commit e808225 into pytorch:master Aug 6, 2020
@mthrok mthrok deleted the torchscript-bc-test branch August 6, 2020 18:37
@mthrok
Copy link
Collaborator Author

mthrok commented Aug 6, 2020

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants