Skip to content

Brew update to fix mac tests #6970

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 8 commits into from
Apr 12, 2021
Merged

Brew update to fix mac tests #6970

merged 8 commits into from
Apr 12, 2021

Conversation

@SeanNaren
Copy link
Contributor

@SeanNaren SeanNaren commented Apr 12, 2021 •
edited by carmocca
Loading

What does this PR do?

Brew installs were failing, updating it seems to fix it however shouldn't be the long term fix. Have a PR to update the image to see if this fixes it #6971

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Apr 12, 2021 •
edited
Loading

Codecov Report

Merging #6970 (9909d13) into master (fe0d088) will decrease coverage by 5%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #6970    +/-   ##
=======================================
- Coverage      92%     87%    -5%     
=======================================
  Files         194     194            
  Lines       12346   12346            
=======================================
- Hits        11331   10694   -637     
- Misses       1015    1652   +637

@SeanNaren SeanNaren changed the title Drop libomp to see what happens Brew update to fix mac tests Apr 12, 2021
@SeanNaren
Copy link
Contributor Author

SeanNaren commented Apr 12, 2021

Hopefully a longer term fix is #6971

@SeanNaren SeanNaren added this to the 1.2.x milestone Apr 12, 2021
@SeanNaren SeanNaren added the bug Something isn't working label Apr 12, 2021
@SeanNaren SeanNaren self-assigned this Apr 12, 2021
@SeanNaren
Copy link
Contributor Author

SeanNaren commented Apr 12, 2021

I'll include the missing changelog in the updated changelog once release is made.

@SeanNaren SeanNaren marked this pull request as ready for review April 12, 2021 17:12
@SeanNaren SeanNaren requested review from Borda and tchaton as code owners April 12, 2021 17:12
@SeanNaren SeanNaren enabled auto-merge (squash) April 12, 2021 17:12
@carmocca carmocca added the ci Continuous Integration label Apr 12, 2021
@carmocca carmocca added the ready PRs ready to be merged label Apr 12, 2021
@SeanNaren SeanNaren disabled auto-merge April 12, 2021 20:53
@SeanNaren SeanNaren enabled auto-merge (squash) April 12, 2021 21:25
Borda
Borda reviewed Apr 12, 2021
View reviewed changes
Borda
Borda reviewed Apr 12, 2021
View reviewed changes
kaushikb11
kaushikb11 reviewed Apr 12, 2021
View reviewed changes
kaushikb11
kaushikb11 reviewed Apr 12, 2021
View reviewed changes
awaelchli
awaelchli approved these changes Apr 12, 2021
View reviewed changes
tests/models/test_horovod.py
Comment on lines +379 to 382
# todo: need to be fixed :]
@pytest.mark.skip(reason="TODO Breaking CI: Aborted (core dumped)")
@RunIf(skip_windows=True, horovod=True)
def test_horovod_multi_optimizer_with_scheduling_stepping(tmpdir):
Copy link
Contributor

@awaelchli awaelchli Apr 12, 2021

Choose a reason for hiding this comment

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

I'll take a look in #6954

@SeanNaren SeanNaren merged commit e9c3e02 into master Apr 12, 2021
@SeanNaren SeanNaren deleted the fix/wip_mac branch April 12, 2021 22:00
@SeanNaren SeanNaren mentioned this pull request Apr 13, 2021
Closed
SeanNaren pushed a commit that referenced this pull request Apr 13, 2021
Brew update to fix mac tests (#6970)
21839a4 
* Drop libomp to see what happens

* Drop openmpi/horovod installs

* Revert "Drop libomp to see what happens"

This reverts commit cdd524f

* Update before install

* Skip horovod failing test

(cherry picked from commit e9c3e02)
awaelchli pushed a commit that referenced this pull request Apr 13, 2021
@awaelchli
Brew update to fix mac tests (#6970)
5d347e9 
* Drop libomp to see what happens

* Drop openmpi/horovod installs

* Revert "Drop libomp to see what happens"

This reverts commit cdd524f

* Update before install

* Skip horovod failing test

(cherry picked from commit e9c3e02)
lexierule pushed a commit that referenced this pull request Apr 14, 2021
@lexierule
Brew update to fix mac tests (#6970)
57142e8 
* Drop libomp to see what happens

* Drop openmpi/horovod installs

* Revert "Drop libomp to see what happens"

This reverts commit cdd524f

* Update before install

* Skip horovod failing test

(cherry picked from commit e9c3e02)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Borda Borda Borda approved these changes

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

+4 more reviewers

@kaushikb11 kaushikb11 kaushikb11 approved these changes

@ananthsub ananthsub ananthsub approved these changes

@awaelchli awaelchli awaelchli approved these changes

@carmocca carmocca carmocca approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@SeanNaren SeanNaren

Labels

bug Something isn't working ci Continuous Integration ready PRs ready to be merged

Projects

None yet

Milestone

pl:1.2.x

Development

Successfully merging this pull request may close these issues.

6 participants

@SeanNaren @ananthsub @awaelchli @Borda @carmocca @kaushikb11