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

Fix parallel buffered slice writer bug #922

Merged
merged 20 commits into from
Aug 14, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9204b09
Extend traceback to get exception of future.
markbader Jun 27, 2023
2bb3883
Implement pad flag for webknossos cli convert subcommand.
markbader Jun 29, 2023
54d3477
Implement hotfix solution for fitting bbox without pad flag.
markbader Jul 10, 2023
3fe785e
Merge branch 'master' into Fix_parallel_BufferedSliceWriter_bug
markbader Jul 10, 2023
7c6f46d
Update changelog.
markbader Jul 10, 2023
44dd88c
Merge branch 'master' into Fix_parallel_BufferedSliceWriter_bug
markbader Jul 13, 2023
52cc6b6
Add update_bbox argument and propagate it from from_images to actual …
markbader Jul 17, 2023
5fe1387
Merge branch 'master' into Fix_parallel_BufferedSliceWriter_bug
markbader Jul 17, 2023
1863194
Implement requested changes.
markbader Jul 17, 2023
3bbcd06
Minor changes to default values.
markbader Jul 17, 2023
a6366c1
Run formatter.
markbader Jul 17, 2023
f230aed
Merge branch 'master' into Fix_parallel_BufferedSliceWriter_bug
markbader Jul 20, 2023
e3336f7
Add filelock dependency and start to change to SoftFileLock implement…
markbader Jul 20, 2023
1f4f773
Implement filelock (upaths are not supported yet).
markbader Jul 20, 2023
9122256
Adapted implementation of SoftFileLock. Still does not support filelo…
markbader Jul 24, 2023
cf643d1
Reverted changes with filelock and make some notes for further implem…
markbader Jul 26, 2023
b21b997
Add json_update_allowed bool.
markbader Aug 1, 2023
54acf99
Merge branch 'master' into Fix_parallel_BufferedSliceWriter_bug
markbader Aug 1, 2023
ea7dc34
Add paragraph in docs and implement requested changes.
markbader Aug 14, 2023
3256aa0
Merge branch 'master' into Fix_parallel_BufferedSliceWriter_bug
markbader Aug 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions webknossos/Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ For upgrade instructions, please check the respective _Breaking Changes_ section
### Changed

### Fixed
- Fixed a bug where parallel access to the properties json leads to an JsonDecodeError in the webknossos CLI [#919](https://github.com/scalableminds/webknossos-libs/issues/919)


## [0.13.1](https://github.com/scalableminds/webknossos-libs/releases/tag/v0.13.1) - 2023-07-17
Expand Down
2 changes: 1 addition & 1 deletion webknossos/webknossos/cli/convert.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ def main(
source,
target,
voxel_size,
name=name,
name,
data_format=data_format,
executor=executor,
compress=compress,
Expand Down
2 changes: 1 addition & 1 deletion webknossos/webknossos/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
upsample,
)

app = typer.Typer(no_args_is_help=True)
app = typer.Typer(no_args_is_help=True, pretty_exceptions_short=False)

app.command("check-equality")(check_equality.main)
app.command("compress")(compress.main)
Expand Down
3 changes: 3 additions & 0 deletions webknossos/webknossos/dataset/_utils/buffered_slice_writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ def __init__(
self,
view: "View",
offset: Optional[Vec3IntLike] = None,
update_bbox: bool = False,
Copy link
Member

Choose a reason for hiding this comment

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

I’d say the default should be True, and there could be a comment explaining what it does and to encourage setting it to false IF parallel access is intended.

# buffer_size specifies, how many slices should be aggregated until they are flushed.
buffer_size: int = 32,
dimension: int = 2, # z
Expand All @@ -48,6 +49,7 @@ def __init__(
self.buffer_size = buffer_size
self.dtype = self.view.get_dtype()
self.use_logging = use_logging
self.update_bbox = update_bbox
if offset is None and relative_offset is None and absolute_offset is None:
relative_offset = Vec3Int.zeros()
if offset is not None:
Expand Down Expand Up @@ -129,6 +131,7 @@ def _write_buffer(self) -> None:
offset=buffer_start.add_or_none(self.offset),
relative_offset=buffer_start_mag1.add_or_none(self.relative_offset),
absolute_offset=buffer_start_mag1.add_or_none(self.absolute_offset),
update_bbox=self.update_bbox,
)

except Exception as exc:
Expand Down
2 changes: 2 additions & 0 deletions webknossos/webknossos/dataset/_utils/pims_images.py
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ def copy_to_view(
args: Tuple[int, int],
mag_view: MagView,
is_segmentation: bool,
update_bbox: bool = False,
dtype: Optional[DTypeLike] = None,
) -> Tuple[Tuple[int, int], Optional[int]]:
"""Copies the images according to the passed arguments to the given mag_view.
Expand All @@ -496,6 +497,7 @@ def copy_to_view(
with mag_view.get_buffered_slice_writer(
relative_offset=(0, 0, z_start * mag_view.mag.z),
buffer_size=mag_view.info.chunk_shape.z,
update_bbox=update_bbox,
) as writer:
for image_slice in images[z_start:z_end]:
image_slice = np.array(image_slice)
Expand Down
9 changes: 6 additions & 3 deletions webknossos/webknossos/dataset/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,7 @@ def add_layer_from_images(
)
mag = mag_view.mag
layer.bounding_box = (
BoundingBox((0, 0, 0), pims_images.expected_shape)
BoundingBox((0, 0, 0), Vec3Int(pims_images.expected_shape))
.from_mag_to_mag1(mag)
.offset(topleft)
)
Expand All @@ -1205,6 +1205,7 @@ def add_layer_from_images(
mag_view=mag_view,
is_segmentation=category == "segmentation",
dtype=current_dtype,
update_bbox=False,
)

args = []
Expand Down Expand Up @@ -1248,9 +1249,11 @@ def add_layer_from_images(
.offset(topleft)
)
if pims_images.expected_shape != actual_size:
warnings.warn(
logger.info(
markbader marked this conversation as resolved.
Show resolved Hide resolved
"[WARNING] Some images are larger than expected, smaller slices are padded with zeros now. "
+ f"New size is {actual_size}, expected {pims_images.expected_shape}.",
+ "New size is %s, expected %s.",
actual_size,
pims_images.expected_shape,
)
if first_layer is None:
first_layer = layer
Expand Down
5 changes: 3 additions & 2 deletions webknossos/webknossos/dataset/mag_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ def write(
self,
data: np.ndarray,
offset: Optional[Vec3IntLike] = None, # deprecated, relative, in current mag
update_bbox: bool = True,
*,
relative_offset: Optional[Vec3IntLike] = None, # in mag1
absolute_offset: Optional[Vec3IntLike] = None, # in mag1
Expand Down Expand Up @@ -177,10 +178,10 @@ def write(

# Only update the layer's bbox if we are actually larger
# than the mag-aligned, rounded up bbox (self.bounding_box):
if not self.bounding_box.contains_bbox(mag1_bbox):
if update_bbox and not self.bounding_box.contains_bbox(mag1_bbox):
self.layer.bounding_box = self.layer.bounding_box.extended_by(mag1_bbox)

super().write(data, absolute_offset=mag1_bbox.topleft)
super().write(data, absolute_offset=mag1_bbox.topleft, update_bbox=update_bbox)

def read(
self,
Expand Down
10 changes: 7 additions & 3 deletions webknossos/webknossos/dataset/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ def write(
self,
data: np.ndarray,
offset: Optional[Vec3IntLike] = None, # deprecated, relative, in current mag
update_bbox: bool = True,
Copy link
Member

Choose a reason for hiding this comment

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

maybe in this context it is rather something like allow_write_outside_view_bbox: bool = False? (this method does not update the bbox, right?)

Copy link
Member

Choose a reason for hiding this comment

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

Also, did you test what happens if there is a write outside of the existing box? does it just work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I agree, that the name allow_write_outside_view_bbox fits better for View.write() but it seems a bit odd to have different signatures for View and MagView.

  2. Yes, it just worked for the files that I used to test it.

*,
relative_offset: Optional[Vec3IntLike] = None, # in mag1
absolute_offset: Optional[Vec3IntLike] = None, # in mag1
Expand Down Expand Up @@ -243,9 +244,10 @@ def write(
abs_mag1_offset=absolute_offset,
current_mag_size=Vec3Int(data.shape[-3:]),
)
assert self.bounding_box.contains_bbox(
mag1_bbox
), f"The bounding box to write {mag1_bbox} is larger than the view's bounding box {self.bounding_box}"
if update_bbox:
assert self.bounding_box.contains_bbox(
mag1_bbox
), f"The bounding box to write {mag1_bbox} is larger than the view's bounding box {self.bounding_box}"

if len(data.shape) == 4 and data.shape[0] == 1:
data = data[0] # remove channel dimension for single-channel data
Expand Down Expand Up @@ -633,6 +635,7 @@ def get_buffered_slice_writer(
offset: Optional[Vec3IntLike] = None,
buffer_size: int = 32,
dimension: int = 2, # z
update_bbox: bool = False,
*,
relative_offset: Optional[Vec3IntLike] = None, # in mag1
absolute_offset: Optional[Vec3IntLike] = None, # in mag1
Expand Down Expand Up @@ -674,6 +677,7 @@ def get_buffered_slice_writer(
return BufferedSliceWriter(
view=self,
offset=offset,
update_bbox=update_bbox,
buffer_size=buffer_size,
dimension=dimension,
relative_offset=relative_offset,
Expand Down