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

Add windows binary jobs #642

Merged
merged 3 commits into from May 16, 2020
Merged

Conversation

peterjc123
Copy link
Contributor

No description provided.

@peterjc123 peterjc123 marked this pull request as ready for review May 15, 2020 06:33
@peterjc123
Copy link
Contributor Author

@mthrok @vincentqb

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #642 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #642   +/-   ##
=======================================
  Coverage   88.84%   88.84%           
=======================================
  Files          21       21           
  Lines        2223     2223           
=======================================
  Hits         1975     1975           
  Misses        248      248           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c28b74...94994ec. Read the comment docs.

@peterjc123
Copy link
Contributor Author

The wheels generated on Windows are noarch. I don't know whether it will break the search process for the wheel packages on Linux/MacOS.

@mthrok mthrok requested a review from seemethere May 15, 2020 14:44
@mthrok
Copy link
Collaborator

mthrok commented May 15, 2020

The wheels generated on Windows are noarch. I don't know whether it will break the search process for the wheel packages on Linux/MacOS.

@seemethere Could you advise?

@peterjc123
Copy link
Contributor Author

peterjc123 commented May 16, 2020

@mthrok I tried to make the wheel package specific both on python version and platform. But I guess it is okay without this change. I quoted the related description from PEP-425.

Sometimes there will be more than one supported built distribution for a particular version of a package. For example, a packager could release a package tagged cp33-abi3-linux_x86_64 that contains an optional C extension and the same distribution tagged py3-none-any that does not. The index of the tag in the supported tags list breaks the tie, and the package with the C extension is installed in preference to the package without because that tag appears first in the list.

@mthrok
Copy link
Collaborator

mthrok commented May 16, 2020

@mthrok I tried to make the wheel package specific both on python version and platform. But I guess it is okay without this change. I quoted the related description from PEP-425.

Thank you for the link.

So based on my understanding of PEP-425, and looking at the job logs, the binaries built are the following. The Windows wheel binaries have win_amd64 as part of the name, so it should not interfere with Linux binaries or macOS binaries. Conda does not have ABI as part of the name but I guess they store such info as meta data somewhere in the files uploaded altogether.

I think it's safe to merge this PR. If this does interfere with binaries for Linux or macOS, then we can tell with the nightly smoke test job tomorrow morning.

Side note:

Is 94994ec sufficient to add ABI for Windows? We plan to revisit and add C++ implementations like #580, #290 which might be able to build on Windows. In that case, we will need to add proper ABI. (though since torchaudio does not build extension on Windows so there are many steps ahead towards that.)


(from the last line of Uploading artifacts job log)

  • Wheel

    • torchaudio-0.6.0.dev20200516-cp38-none-win_amd64.whl
      • Python: CPython3.8
      • ABI: N/A
      • Platform: win_amd64
    • torchaudio-0.6.0.dev20200515-cp38-cp38-macosx_10_9_x86_64.whl
      • Python: CPython3.8
      • ABI: CPython3.8
      • Platform: macosx_10_9_x86_64
    • torchaudio-0.6.0.dev20200516-cp38-cp38-linux_x86_64.whl
      • Python: CPython3.8
      • ABI: CPython3.8
      • Platform: linux_x86_64
  • Conda

    • win-64\torchaudio-0.6.0.dev20200516-py38.tar.bz2
      • Python: Python 3.8
      • ABI: N/A
      • Platform: win-64
    • osx-64/torchaudio-0.6.0.dev20200515-py38.tar.bz2
      • Python: Python 3.8
      • ABI: cpython-38-darwin
      • Platform: osx-64
    • linux-64/torchaudio-0.6.0.dev20200516-py38.tar.bz2
      • Python: Python 3.8
      • ABI: cpython-38-x86_64-linux-gnu
      • Platform: linux-64

@mthrok mthrok merged commit e3a4708 into pytorch:master May 16, 2020
@peterjc123 peterjc123 deleted the add_windows_binary_jobs branch May 16, 2020 02:06
bhargavkathivarapu pushed a commit to bhargavkathivarapu/audio that referenced this pull request May 19, 2020
* Add windows binary jobs

* Make wheel package platform-specific and python-version-specific
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.

None yet

2 participants