Skip to content

Parallelise operations - #51

Closed
benjeffery-ofh wants to merge 1 commit into
sgkit-dev:mainfrom
benjeffery-ofh:parallel
Closed

benjeffery-ofh wants to merge 1 commit into
sgkit-dev:mainfrom
benjeffery-ofh:parallel

Conversation

@benjeffery-ofh

Copy link
Copy Markdown
Contributor

WIP

@benjeffery-ofh

Copy link
Copy Markdown
Contributor Author

Initial findings are that this is 200x faster for the copy up operation

@tomwhite tomwhite left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work on speeding things up! I had a few preliminary comments.

In addition to the asyncio changes, this adds CLI tests, and checks variant chunk alignment - both of which are orthogonal to the main change. Would it be possible to break up the changes?

Comment thread tests/test_cli.py
assert seen["io_concurrency"] == 3


def test_old_copy_concurrency_flag_is_removed():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure this test is needed?

Comment thread vczstore/normalise.py
return array


def _assert_variant_chunk_alignment(arrays, *, variant_chunk_size, operation):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be possible to make this function work for all operations - given a set of target arrays check they all have the given chunking.

Comment thread vczstore/concurrency.py
import inspect
import os

DEFAULT_IO_CONCURRENCY = 32

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the rationale for this number? Should it be a small factor times the number of cores on the machine (like in bio2zarr)?

@tomwhite

Copy link
Copy Markdown
Member

Hi @benjeffery-ofh - do you have any updates to this PR? It would be good to get it merged (or at least parts of it), as it's a big change and I've got a series of changes I'd like to make that will conflict.

@benjeffery-ofh

Copy link
Copy Markdown
Contributor Author

I'll break this out into the separate PRs

@tomwhite

Copy link
Copy Markdown
Member

Thanks!

This was referenced Apr 28, 2026
@benjeffery-ofh

Copy link
Copy Markdown
Contributor Author

Closed in favour of smaller PRs

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