Skip to content

Add windows ci build #1049

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 29, 2020
Merged

Add windows ci build #1049

merged 1 commit into from
Jul 29, 2020

Conversation

guyang3532
Copy link
Contributor

@guyang3532 guyang3532 commented Jul 3, 2020 •
edited
Loading

No description provided.

@netlify
Copy link

netlify bot commented Jul 3, 2020

Deploy preview for pytorch-tutorials-preview ready!

Built with commit 83b580c

https://deploy-preview-1049--pytorch-tutorials-preview.netlify.app

@netlify
Copy link

netlify bot commented Jul 3, 2020

Deploy preview for pytorch-tutorials-preview ready!

Built with commit 8f3a30d

https://deploy-preview-1049--pytorch-tutorials-preview.netlify.app

@netlify
Copy link

netlify bot commented Jul 3, 2020 •
edited
Loading

Deploy preview for pytorch-tutorials-preview ready!

Built with commit 618430e

https://deploy-preview-1049--pytorch-tutorials-preview.netlify.app

@guyang3532 guyang3532 force-pushed the test_win_build branch 22 times, most recently from 2ace553 to c0eda34 Compare July 22, 2020 04:55
@guyang3532 guyang3532 changed the title [don't merge]test for windows build Add windows ci build Jul 22, 2020
seemethere
seemethere requested changes Jul 22, 2020
View reviewed changes
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.

I don't think we can allow this to run on every single PR and every single merge for that matter, it just seems too expensive in what it actually costs to run for us to do that

.circleci/config.yml Outdated
Comment on lines 586 to 601
- pytorch_windows_build_worker_0:
name: win_test_worker_0
- pytorch_windows_build_worker_1:
name: win_test_worker_1
- pytorch_windows_build_worker_2:
name: win_test_worker_2
- pytorch_windows_build_worker_3:
name: win_test_worker_3
- pytorch_windows_build_worker_4:
name: win_test_worker_4
- pytorch_windows_build_worker_5:
name: win_test_worker_5
- pytorch_windows_build_worker_6:
name: win_test_worker_6
- pytorch_windows_build_worker_7:
name: win_test_worker_7
Copy link
Member

Choose a reason for hiding this comment

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

This job seems extremely expensive to run basically the way the math works this would be 500 credits x 55 minutes * 8 workers

We should at the very least gate these to run only on master and even, I think, gate these to only run when someone manually approves these to run.

cc @malfet, @yns88

Copy link

Choose a reason for hiding this comment

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

Does every worker need a GPU machine to run? Can some of the work be shifted to CPU-only machines? Can the amount of coverage on the Windows build be reduced so it doesn't need 8 workers?

If not, then yes, we'll need to limit this to master-only (that might be sufficient, since this repository isn't nearly as active as pytorch/pytorch).

Copy link
Contributor Author

@guyang3532 guyang3532 Jul 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Considering the parallel jobs (8 workers with most 55minutes for one) seems not much more efficient than one job (1 hour 36 minutes). I have changed the parallel jobs to a single one only on master branch.
@seemethere @yns88

.circleci/config.yml Outdated
Comment on lines 586 to 601
- pytorch_windows_build_worker_0:
name: win_test_worker_0
- pytorch_windows_build_worker_1:
name: win_test_worker_1
- pytorch_windows_build_worker_2:
name: win_test_worker_2
- pytorch_windows_build_worker_3:
name: win_test_worker_3
- pytorch_windows_build_worker_4:
name: win_test_worker_4
- pytorch_windows_build_worker_5:
name: win_test_worker_5
- pytorch_windows_build_worker_6:
name: win_test_worker_6
- pytorch_windows_build_worker_7:
name: win_test_worker_7
Copy link
Member

Choose a reason for hiding this comment

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

Here's what a manual approval would look like:

Suggested change
- pytorch_windows_build_worker_0:
name: win_test_worker_0
- pytorch_windows_build_worker_1:
name: win_test_worker_1
- pytorch_windows_build_worker_2:
name: win_test_worker_2
- pytorch_windows_build_worker_3:
name: win_test_worker_3
- pytorch_windows_build_worker_4:
name: win_test_worker_4
- pytorch_windows_build_worker_5:
name: win_test_worker_5
- pytorch_windows_build_worker_6:
name: win_test_worker_6
- pytorch_windows_build_worker_7:
name: win_test_worker_7
- hold:
type: approval
- pytorch_windows_build_worker_0:
name: win_test_worker_0
requires:
- hold
- pytorch_windows_build_worker_1:
name: win_test_worker_1
requires:
- hold
- pytorch_windows_build_worker_2:
name: win_test_worker_2
requires:
- hold
- pytorch_windows_build_worker_3:
name: win_test_worker_3
requires:
- hold
- pytorch_windows_build_worker_4:
name: win_test_worker_4
requires:
- hold
- pytorch_windows_build_worker_5:
name: win_test_worker_5
requires:
- hold
- pytorch_windows_build_worker_6:
name: win_test_worker_6
requires:
- hold
- pytorch_windows_build_worker_7:
name: win_test_worker_7
requires:
- hold

Makefile Outdated
@@ -45,18 +45,18 @@ download:
mkdir -p intermediate_source/data

# transfer learning tutorial data
wget -N https://download.pytorch.org/tutorial/hymenoptera_data.zip -P $(DATADIR)
wget -N http://download.pytorch.org/tutorial/hymenoptera_data.zip -P $(DATADIR)
Copy link
Member

Choose a reason for hiding this comment

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

Why the switch from https -> http?

I'd much prefer it to use https over regular http?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the switch from https -> http?

I'd much prefer it to use https over regular http?

fixed

@seemethere seemethere requested a review from malfet July 22, 2020 17:17
@seemethere seemethere requested a review from yns88 July 22, 2020 17:17
@guyang3532 guyang3532 force-pushed the test_win_build branch 4 times, most recently from 04deb28 to 0fc7e4f Compare July 28, 2020 12:38
@guyang3532 guyang3532 force-pushed the test_win_build branch 2 times, most recently from 502ee0d to a86c122 Compare July 28, 2020 15:52
@guyang3532
Copy link
Contributor Author

The CI failure seems not related to this change.

@guyang3532 guyang3532 requested a review from seemethere July 29, 2020 16:26
@seemethere
Copy link
Member

I've re-kicked the failed jobs since they appear to be timeouts, will merge on green

@seemethere seemethere merged commit b97599a into pytorch:master Jul 29, 2020
@brianjo
Copy link
Contributor

brianjo commented Jul 30, 2020

The CI for this PR failed. I've kicked the build on the win thread that failed.

brianjo added a commit that referenced this pull request Jul 30, 2020
@brianjo
Revert "Add windows ci build (#1049)"
0b72a91 
This reverts commit b97599a.
@brianjo brianjo mentioned this pull request Jul 30, 2020
Merged
brianjo added a commit that referenced this pull request Jul 31, 2020
@brianjo
Revert "Add windows ci build (#1049)" (#1094)
bc9cac0 
This reverts commit b97599a.
guyang3532 added a commit to guyang3532/tutorials that referenced this pull request Aug 3, 2020
@guyang3532
Add windows ci build (pytorch#1049)
8dab164
guyang3532 added a commit to guyang3532/tutorials that referenced this pull request Aug 4, 2020
@guyang3532
Add windows ci build (pytorch#1049)
84cec25
brianjo pushed a commit that referenced this pull request Aug 4, 2020
@guyang3532
Add windows ci build (#1049) (#1102)
cca97f0
rodrigo-techera pushed a commit to Experience-Monks/tutorials that referenced this pull request Nov 29, 2021
@guyang3532
Add windows ci build (pytorch#1049)
79c0f45
rodrigo-techera pushed a commit to Experience-Monks/tutorials that referenced this pull request Nov 29, 2021
@brianjo
Revert "Add windows ci build (pytorch#1049)" (pytorch#1094)
16ec555 
This reverts commit b97599a.
rodrigo-techera pushed a commit to Experience-Monks/tutorials that referenced this pull request Nov 29, 2021
@guyang3532
Add windows ci build (pytorch#1049) (pytorch#1102)
bdadb90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seemethere seemethere seemethere approved these changes

@malfet malfet malfet approved these changes

@yns88 yns88 Awaiting requested review from yns88

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@guyang3532 @seemethere @brianjo @yns88 @malfet