Skip to content

Conversation

@clee2000
Copy link
Contributor

@clee2000 clee2000 commented Apr 14, 2022

Fixes #ISSUE_NUMBER

tested via #75232 b/c need to change the source of the workflow

  • set fetch-depth: 1
  • manually checkout additional branches/history (usually either viable/strict, or master and the rest of the commit's history) when needed
  • seems to reduce checkout time by about 30s for jobs that don't need additional branches/history, but minimal improvement otherwise
    • checkouts for most lint jobs now takes <15s

Rough estimates for how long different parts of checkout take on linux (windows is similar, but scaled up):

  • just the commit, no history: <15s, seems to be around 6-7s
  • viable/strict: 25-30s
  • submodules: 80-120s
  • master + commit history: 40-50s (if checked out viable/strict before this, then this time is much smaller, <10s)

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 14, 2022

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 91a40b0 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot facebook-github-bot added the module: rocm AMD GPU support for Pytorch label Apr 14, 2022
@clee2000 clee2000 marked this pull request as ready for review April 14, 2022 15:50
@clee2000 clee2000 marked this pull request as draft April 14, 2022 15:53
@clee2000 clee2000 marked this pull request as ready for review April 14, 2022 16:41
# deep clone, to allow use of git merge-base
fetch-depth: 0
# --depth=1 for speed, manually fetch history and other refs in workflows as necessary
fetch-depth: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@seemethere seemethere left a comment

Choose a reason for hiding this comment

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

Mostly LGTM but seems like a good opportunity to move some of the repeated code blocks into composite actions to make them easier to update later on

- name: Checkout rest of history and master
env:
HASH: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.sha) || github.sha }}
run: git fetch --no-tags --prune --no-recurse-submodules --unshallow origin "${HASH}" master
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to make these repeated code blocks just composite actions?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 could these be rolled into the checkout pytorch composite action?

@clee2000 clee2000 force-pushed the clee2000/fetch-depth branch from 6444e9f to 1b73e0b Compare April 15, 2022 02:30
@clee2000 clee2000 force-pushed the clee2000/fetch-depth branch from 606c46a to 3837900 Compare April 15, 2022 16:46
Copy link
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

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

woooooooo~

@janeyx99
Copy link
Contributor

Though I think you should change the PR title cuz you're doing the opposite

@clee2000 clee2000 changed the title set fetch-depth: 0 set fetch-depth: 1 Apr 15, 2022
@clee2000 clee2000 added the topic: not user facing topic category label Apr 15, 2022
@clee2000
Copy link
Contributor Author

@pytorchbot merge this please

This was referenced Apr 18, 2022
pytorchmergebot pushed a commit that referenced this pull request Apr 18, 2022
Fixes #ISSUE_NUMBER

Follow up to #75783

Inputs are always strings, and the string wasn't empty, so the if statement always evaluated to true, so it always checked out the extra branches. Add check that the input is literally 'true' in order to checkout the extra branches

tested via #75955
Pull Request resolved: #75957
Approved by: https://github.com/seemethere
facebook-github-bot pushed a commit that referenced this pull request Apr 18, 2022
Summary:
Fixes #ISSUE_NUMBER

tested via #75232 b/c need to change the source of the workflow
- set fetch-depth: 1
- manually checkout additional branches/history (usually either viable/strict, or master and the rest of the commit's history) when needed
- seems to reduce checkout time by about 30s for jobs that don't need additional branches/history, but minimal improvement otherwise
  - checkouts for most lint jobs now takes <15s

Rough estimates for how long different parts of checkout take on linux (windows is similar, but scaled up):
- just the commit, no history: <15s, seems to be around 6-7s
- viable/strict: 25-30s
- submodules: 80-120s
- master + commit history: 40-50s (if checked out viable/strict before this, then this time is much smaller, <10s)

Pull Request resolved: #75783
Approved by: https://github.com/seemethere, https://github.com/janeyx99

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/991c89b2d12ce46d618969767b931016e049fecb

Reviewed By: mehtanirav

Differential Revision: D35721631

Pulled By: clee2000

fbshipit-source-id: e7dc833a34caa64e81c3b1c67124f78cd4ef74d7
facebook-github-bot pushed a commit that referenced this pull request Apr 19, 2022
Summary:
Fixes #ISSUE_NUMBER

Follow up to #75783

Inputs are always strings, and the string wasn't empty, so the if statement always evaluated to true, so it always checked out the extra branches. Add check that the input is literally 'true' in order to checkout the extra branches

tested via #75955

Pull Request resolved: #75957
Approved by: https://github.com/seemethere

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/cdbd39ba5714d37398c261a283ede26754e1025e

Reviewed By: seemethere

Differential Revision: D35751449

Pulled By: clee2000

fbshipit-source-id: e8afbdaceffe056827f484d3c6ad3ece5eb96452
@clee2000 clee2000 mentioned this pull request Apr 20, 2022
pytorchmergebot pushed a commit that referenced this pull request Apr 20, 2022
Fixes #ISSUE_NUMBER
undo #75783 b/c setting fetch depth 1 doesn't really help reduce time b/c most of the jobs need either master or viable/strict

also, more branches need viable/strict than i thought, so sharding isn't picking up test times (although default sharding seems to do pretty well) (regarding the jobs i didn't realize needed viable/strict: it looks like the linux-bionic jobs don't fail when `git rev-parse viable/strict` is run but viable/strict doesn't exist but the linux-xenial ones do)

pretty sure jobs are broken only b/c its using the master version of `checkout-pytorch/action.yml`
tested via #76077
Pull Request resolved: #76090
Approved by: https://github.com/seemethere
malfet pushed a commit that referenced this pull request Apr 20, 2022
Fixes #ISSUE_NUMBER

Follow up to #75783

Inputs are always strings, and the string wasn't empty, so the if statement always evaluated to true, so it always checked out the extra branches. Add check that the input is literally 'true' in order to checkout the extra branches

tested via #75955
Pull Request resolved: #75957
Approved by: https://github.com/seemethere

(cherry picked from commit cdbd39b)
malfet pushed a commit that referenced this pull request Apr 20, 2022
Fixes #ISSUE_NUMBER
undo #75783 b/c setting fetch depth 1 doesn't really help reduce time b/c most of the jobs need either master or viable/strict

also, more branches need viable/strict than i thought, so sharding isn't picking up test times (although default sharding seems to do pretty well) (regarding the jobs i didn't realize needed viable/strict: it looks like the linux-bionic jobs don't fail when `git rev-parse viable/strict` is run but viable/strict doesn't exist but the linux-xenial ones do)

pretty sure jobs are broken only b/c its using the master version of `checkout-pytorch/action.yml`
tested via #76077
Pull Request resolved: #76090
Approved by: https://github.com/seemethere

(cherry picked from commit 5477f0a)
facebook-github-bot pushed a commit that referenced this pull request Apr 21, 2022
Summary:
Fixes #ISSUE_NUMBER
undo #75783 b/c setting fetch depth 1 doesn't really help reduce time b/c most of the jobs need either master or viable/strict

also, more branches need viable/strict than i thought, so sharding isn't picking up test times (although default sharding seems to do pretty well) (regarding the jobs i didn't realize needed viable/strict: it looks like the linux-bionic jobs don't fail when `git rev-parse viable/strict` is run but viable/strict doesn't exist but the linux-xenial ones do)

pretty sure jobs are broken only b/c its using the master version of `checkout-pytorch/action.yml`
tested via #76077

Pull Request resolved: #76090
Approved by: https://github.com/seemethere

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/5477f0ae60d40f3fcc138a0292608afd753c8569

Reviewed By: seemethere

Differential Revision: D35785884

Pulled By: clee2000

fbshipit-source-id: 66e3bf8b1fec837632e416c49e85b78e6d7ebc5f
@clee2000 clee2000 deleted the clee2000/fetch-depth branch May 16, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed module: rocm AMD GPU support for Pytorch topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants