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

Reuse common logic and migrate CUDA >= 10.1 jobs to VS 2019 for Windo… #2264

Merged
merged 8 commits into from
May 29, 2020

Conversation

peterjc123
Copy link
Contributor

…ws jobs

@peterjc123 peterjc123 requested review from ezyang and fmassa May 27, 2020 08:19
@peterjc123 peterjc123 marked this pull request as ready for review May 27, 2020 11:38
@peterjc123 peterjc123 requested a review from seemethere May 27, 2020 15:55
@peterjc123
Copy link
Contributor Author

peterjc123 commented May 27, 2020

@fmassa Hi, with this code refactor PR, I think it will be better to split out the unit test so that they don't run in binary jobs. And also we should migrate the unit test jobs to CircleCI. This is already done in pytorch/audio and pytorch/text. What do you think?

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.

Some minor comments, but otherwise LGTM

steps:
- checkout_merge
- run:
command: |
choco install miniconda3
(& "C:\tools\miniconda3\Scripts\conda.exe" "shell.powershell" "hook") | Out-String | Invoke-Expression
set -ex
Copy link
Member

Choose a reason for hiding this comment

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

This might be a follow up PR but it'd be nice if this logic was just captured in a re-usable command or a script so that we don't have to copy / paste it everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Will fix.

set VC_VERSION_UPPER=16
)

for /f "usebackq tokens=*" %%i in (`"%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe" -legacy -products * -version [%VC_VERSION_LOWER%^,%VC_VERSION_UPPER%^) -property installationPath`) do (
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to condense this as an extra step in the circleci config that runs only on windows wheel builds?

Seems like it'd be better to do that instead of having it completely shell out to a separate script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not easy to share an environment between Bash and CMD, especially when you want to activate the env in CMD and get the env back in Bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I come up with an idea that we could create a utility to activate the env and do sth in CMD by passing arguments.

@peterjc123
Copy link
Contributor Author

@seemethere Comments fixed.

@fmassa
Copy link
Member

fmassa commented May 29, 2020

Hi, with this code refactor PR, I think it will be better to split out the unit test so that they don't run in binary jobs. And also we should migrate the unit test jobs to CircleCI. This is already done in pytorch/audio and pytorch/text. What do you think?

Yes, this is something that would be great to do. I did it as is for Linux / OSX because it was the simplest way to get things working, but it should be separated ideally.

@seemethere
Copy link
Member

Interesting why the windows builds are failng, perhaps it's due to the fact that the windows conda uploads have been broken upstream?

@peterjc123
Copy link
Contributor Author

peterjc123 commented May 29, 2020

@seemethere Yes, the upstream pytorch binary jobs are failing. Have to wait for the upstream nightly conda packages to be uploaded.

@seemethere
Copy link
Member

seemethere commented May 29, 2020

Other conda CUDA versions have passed, I'm comfortable merging this as is.

@seemethere seemethere merged commit df8d776 into pytorch:master May 29, 2020
@peterjc123 peterjc123 deleted the migrate_vs2019 branch May 30, 2020 09:20
@peterjc123
Copy link
Contributor Author

Yes, this is something that would be great to do. I did it as is for Linux / OSX because it was the simplest way to get things working, but it should be separated ideally.

The MS folks @nbcsm @guyang3532 are working on this.

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.

3 participants