Skip to content

Toggle on/off ffmpeg test if needed #2901

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

Closed
wants to merge 7 commits into from

Conversation

atalman
Copy link
Contributor

@atalman atalman commented Dec 8, 2022

Toggle on/off ffmpeg test if needed
By default it ON, hence should not affect any current tests.
To toggle ON no change required.
To toggle OFF use:

smoke_test.py --no-ffmpeg

To be used when calling from builder currently. Since we do not install ffmpeg currently.

@atalman atalman changed the title Toggle streamreader test if needed Toggle on/off streamreader test if needed Dec 8, 2022
Comment on lines 20 to 21
parser.add_argument("--streamreader", action="store_true")
parser.add_argument("--no-streamreader", dest="streamreader", action="store_false")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be achieved w/ just one argument

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, can we just have one argument --no-streamreader so that it defaults to test streamreader.

Also, I think it's more correct to say --no-ffmpeg instead of streamreader as there is streamwriter as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mthrok
Copy link
Collaborator

mthrok commented Dec 8, 2022

This is very sensitive change, and I am not in favor of this, because the reason this test was added is to makes sure that FFmpeg installation (a version different from the one used for build) is properly integrated, which constitutes the most basic smoke test.

If this option is abused, the binary being tested will be in a bad shape.
Can you elaborate on usecases?

@atalman
Copy link
Contributor Author

atalman commented Dec 8, 2022

This is very sensitive change, and I am not in favor of this, because the reason this test was added is to makes sure that FFmpeg installation (a version different from the one used for build) is properly integrated, which constitutes the most basic smoke test.

If this option is abused, the binary being tested will be in a bad shape. Can you elaborate on usecases?

This is to be used when calling from https://pytorch.org/get-started/locally/ and release validation workflows in https://github.com/pytorch/builder. Which do not install ffmpeg currently.

def main() -> None:
parser = argparse.ArgumentParser()
parser.add_argument("--streamreader", action="store_true")
parser.add_argument("--no-streamreader", dest="streamreader", action="store_false")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, can we have some warning and note on this option?
The use of this argument should restricted to minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@atalman atalman changed the title Toggle on/off streamreader test if needed Toggle on/off ffmpeg test if needed Dec 9, 2022
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@atalman merged this pull request in ccda545.

@github-actions
Copy link

github-actions bot commented Dec 9, 2022

Hey @atalman.
You merged this PR, but labels were not properly added. Please add a primary and secondary label (See https://github.com/pytorch/audio/blob/main/.github/process_commit.py)

BriansIDP pushed a commit to BriansIDP/audio that referenced this pull request Dec 12, 2022
Summary:
Toggle on/off ffmpeg test if needed
By default it ON, hence should not affect any current tests.
To toggle ON no change required.
To toggle OFF use:
```
smoke_test.py --no-ffmpeg
```

To be used when calling from builder currently. Since we do not install ffmpeg currently.

Pull Request resolved: pytorch#2901

Reviewed By: carolineechen, mthrok

Differential Revision: D41874976

Pulled By: atalman

fbshipit-source-id: c57b19f37c63a1f476f93a5211550e980e67d9c7
mthrok pushed a commit to mthrok/audio that referenced this pull request Jan 17, 2023
Summary:
Toggle on/off ffmpeg test if needed
By default it ON, hence should not affect any current tests.
To toggle ON no change required.
To toggle OFF use:
```
smoke_test.py --no-ffmpeg
```

To be used when calling from builder currently. Since we do not install ffmpeg currently.

Pull Request resolved: pytorch#2901

Reviewed By: carolineechen, mthrok

Differential Revision: D41874976

Pulled By: atalman

fbshipit-source-id: c57b19f37c63a1f476f93a5211550e980e67d9c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants