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

Revert "Improved fill_halo_regions" #1484

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

ali-ramadhan
Copy link
Member

Reverts #1444

PR #1444 is quite nice but unfortunately I think the new fill_halo_regions!

fill_halo_regions!(c::AbstractArray, fieldbcs, grid, args...; kwargs...)

broke MPI halo filling since it needs dispatch on

fill_halo_regions!(c::AbstractArray, bcs, arch::AbstractMultiArchitecture, grid, c_location, args...)

It might be tricky to fix since you can't tell whether you're on CPU or MultiCPU from inside fill_halo_regions! with just knowledge of the field...

I propose we revert #1444 just for now (since distributed models + tests are broken on master) and immediately open a new PR with the contents of #1444 where we can find a solution. My only idea is to add architecture to Field but perhaps it feels a little bloated...

This failure was not caught by Buildkite since PR #590 was merged after #1444 was opened, and I think Buildkite only runs tests on branches, not pull requests (i.e. it doesn't merge master and run tests).

@ali-ramadhan
Copy link
Member Author

The implicit free surface solver tests are also failing on the main branch: https://buildkite.com/clima/oceananigans/builds/1644 but this seems like it would be a trivial fix.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

:-O

@glwagner
Copy link
Member

It seems reasonable to add architecture to AbstractField. I think we actually might need this to make AbstractOperations work generically on distributed systems (in particular, AbstractOperations that depend on Fields or ComputedFields).

@glwagner
Copy link
Member

(Because in compute!, we already try to infer architecture from field.data...)

@ali-ramadhan
Copy link
Member Author

Ah cool, so adding architecture to field seems like it might be good overall.

I'll merge and open a new PR with the contents of #1444.

@ali-ramadhan ali-ramadhan merged commit 9b52f3f into master Mar 16, 2021
@ali-ramadhan ali-ramadhan deleted the revert-1444-glw/improve-halo-filling branch March 16, 2021 19:42
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.

2 participants