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

[Easy] 5-minute timeout for smoke test tutorials #1707

Closed
wants to merge 2 commits into from

Conversation

esantorella
Copy link
Member

Motivation

Currently, all tutorials time out at 30 minutes. We want smoke test tutorials to take much less time than that, and currently they all run in under 3.5 minutes, and most run much faster than that (see here). As we work to get more tutorials online, it would be good to a have a clear idea of what is fast enough for a smoke-test tutorial run.

Test Plan

Checked that it ran locally; unit tests

@esantorella esantorella self-assigned this Feb 27, 2023
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Feb 27, 2023
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@saitcakmak saitcakmak left a comment

Choose a reason for hiding this comment

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

A small fix in-line, LGTM otherwise!

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #1707 (4ee4d4b) into main (587885d) will not change coverage.
The diff coverage is n/a.

❗ Current head 4ee4d4b differs from pull request most recent head 886e86e. Consider uploading reports for the commit 886e86e to get more accurate results

@@            Coverage Diff            @@
##              main     #1707   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          170       170           
  Lines        14610     14610           
=========================================
  Hits         14610     14610           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Co-authored-by: Sait Cakmak <saitcakmak@outlook.com>
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@esantorella merged this pull request in 10b35f9.

facebook-github-bot pushed a commit that referenced this pull request Feb 28, 2023
Summary:
## Motivation
Some tutorials were being ignored both in "smoke test" and "standard" runs because they OOM or time out in standard runs, but they actually run fine in "smoke test" mode. I moved those three tutorials to the list now called `RUN_IF_SMOKE_TEST_IGNORE_IF_STANDARD` (renamed for clarity).

Pull Request resolved: #1708

Test Plan:
Ran locally; should run in CI

## Related PRs

Depends on #1707 (add five-minute timeout for smoke test tutorials)

Reviewed By: saitcakmak

Differential Revision: D43637457

Pulled By: esantorella

fbshipit-source-id: 6f5f1f02bd381f552944763ba8f697cc2cafc8a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants