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

Adding MacOS unit tests on CircleCI #1672

Merged
merged 8 commits into from Apr 4, 2022

Conversation

Nayef211
Copy link
Contributor

@Nayef211 Nayef211 commented Apr 1, 2022

Reference Issue #1644

Description

  • Enables MacOS unit tests on CircleCI

Testing

  • Ensure all MacOS tests pass on CircleCI

@Nayef211 Nayef211 changed the title Added macos unittests on circle ci [WIP] Adding MacOS unit tests on CircleCI Apr 1, 2022
@mthrok
Copy link
Contributor

mthrok commented Apr 1, 2022

FYI: The setup script uses yaml file to list the environment.

https://github.com/pytorch/text/blob/main/.circleci/unittest/linux/scripts/setup_env.sh#L39
https://github.com/pytorch/text/blob/main/.circleci/unittest/linux/scripts/environment.yml

This seems like a clean way to install the same set of packages, but it was disaster for macOS conda environments so audio stopped doing it. (Always getting UnsatisfiableDependencies error)
You might encounter the same situation.

@Nayef211
Copy link
Contributor Author

Nayef211 commented Apr 1, 2022

FYI: The setup script uses yaml file to list the environment.

https://github.com/pytorch/text/blob/main/.circleci/unittest/linux/scripts/setup_env.sh#L39 https://github.com/pytorch/text/blob/main/.circleci/unittest/linux/scripts/environment.yml

This seems like a clean way to install the same set of packages, but it was disaster for macOS conda environments so audio stopped doing it. (Always getting UnsatisfiableDependencies error) You might encounter the same situation.

I see so what was the workaround that you use in torchaudio to resolve this?

@mthrok
Copy link
Contributor

mthrok commented Apr 1, 2022

FYI: The setup script uses yaml file to list the environment.
https://github.com/pytorch/text/blob/main/.circleci/unittest/linux/scripts/setup_env.sh#L39 https://github.com/pytorch/text/blob/main/.circleci/unittest/linux/scripts/environment.yml
This seems like a clean way to install the same set of packages, but it was disaster for macOS conda environments so audio stopped doing it. (Always getting UnsatisfiableDependencies error) You might encounter the same situation.

I see so what was the workaround that you use in torchaudio to resolve this?

Manually listing them.

https://github.com/pytorch/audio/blob/87f0d1989014f2da9d56a326b54507a7a1cbc712/.circleci/unittest/linux/scripts/install.sh#L72-L73

I wonder why YAML approach wouldn't work the same way... Maybe we had mis understanding of the situation. Because even with this approach, macOS conda dependencies are broken most of the time.

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #1672 (bef3caa) into main (c8ac763) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1672   +/-   ##
=======================================
  Coverage   89.29%   89.29%           
=======================================
  Files          51       51           
  Lines        2298     2298           
=======================================
  Hits         2052     2052           
  Misses        246      246           

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 c8ac763...bef3caa. Read the comment docs.

Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Looks good. I see green light on macOS tests, but just to be sure skim through the test log to see if there is any abnormality that are suppressed.

test/experimental/test_with_asset.py Outdated Show resolved Hide resolved
test/experimental/test_with_asset.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Nayef211 Nayef211 left a comment

Choose a reason for hiding this comment

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

Thanks @mthrok for the review and help in adding the tests. I just looked through the MacOS test logs and everything looked okay. Will merge this once all the CI jobs pass

test/experimental/test_with_asset.py Outdated Show resolved Hide resolved
test/experimental/test_with_asset.py Outdated Show resolved Hide resolved
@Nayef211 Nayef211 changed the title [WIP] Adding MacOS unit tests on CircleCI Adding MacOS unit tests on CircleCI Apr 4, 2022
@Nayef211 Nayef211 merged commit b710c88 into pytorch:main Apr 4, 2022
@Nayef211 Nayef211 deleted the feature/enable_unittest_macos branch April 4, 2022 20:34
@mthrok mthrok mentioned this pull request Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants