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

Consolidate concat_dim earlier in prepare_target #229

Closed

Conversation

TomAugspurger
Copy link
Contributor

@TomAugspurger TomAugspurger commented Nov 9, 2021

Closes #227
(maybe, might be worth testing).

A few notes:

  1. I'm not really happy with how I'm handling the locking (see https://github.com/pangeo-forge/pangeo-forge-recipes/pull/229/files#diff-e12c886cc124886c5cfa5313d760a36c39649af9da845077c663e6feab8487b5R245-R253). I just kind of overwrite what the "real" lock determination code does. This feels too special-casey, but I haven't dug into the how that conflict code works.
  2. I haven't tested this at scale yet. Maybe @sharkinsspatial can help verify that this actually fixes things? Let me know if you're able to, or if I should get something set up.

# https://github.com/pangeo-forge/pangeo-forge-recipes/issues/214
# intersect the dims from the array metadata with the Zarr group
# to handle coordinateless dimensions.
def _consolidate_dimension_coordinate(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff here looks strange.

All I did was

  1. move the if config.consolidate_dimension_coordinates to its own function so that I could call it from two places (here and earlier in prepare_target for concat_dim)
  2. Let the caller pass in the dims to check (so that we can do just concat_dim early on, and the other dims later; I think this is what we want).
  3. Added a check that arr.chunks != arr.shape (to avoid rewriting concat_diminfinalize_target`, since it would already be consolidated)

@sharkinsspatial
Copy link
Contributor

@TomAugspurger I've been locked up on other things today but I'll try to run some testing with this tomorrow 👍

@rabernat
Copy link
Contributor

/run-test-tutorials

@TomAugspurger
Copy link
Contributor Author

As discovered by @sharkinsspatial, this implementation buggy (sorry). When we call resize, we end up with a "fragmented" array that has 360,000 entries and a chunksize of 1. So it's already too late. I'll explore our options (and the CI failure)

@rabernat
Copy link
Contributor

@TomAugspurger - over in #236, I have removed the pre-initialization of the concat dim with 0 values. It doesn't seem to break anything (including the tutorial notebooks). If that is the main bottleneck, I would prefer that (simpler) solution over this (more complex) one.

I think the real fix for #227 is upstream in fsspec, related by supporting batch_size more broadly across async operations.

@rabernat
Copy link
Contributor

p.s. If you plan to keep working on this branch, please rebase as there have been some big improvements to the test suite.

@cisaacstern
Copy link
Member

Closing as stale/out-of-date. Thanks all!

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.

Debugging memory issues in IMERG
4 participants