mobie / mobie-utils-python

Python tools for MoBIE
MIT License
9 stars 5 forks source link

`add_bdv_image` minimum input, validation error #44

Closed martinschorb closed 2 years ago

martinschorb commented 2 years ago

Hi,

I tried to use add_bdv_image with minimal input to include an existing n5 into a MoBIE project. I provided the xml path, output folder and dataset name as minimal (positional and thus required) parameters.

It fails during validation because something with the view seems wrong.

mobie.add_bdv_image(im_path, mobie_project_folder, "data1")

File /g/emcf/schorb/code/mobie-utils-python/mobie/image_data.py:109, in add_bdv_image(xml_path, root, dataset_name, image_name, file_format, menu_name, scale_factors, tmp_folder, target, max_jobs, is_default_dataset, description, trafos_for_mobie)
    105 view, transformation = _view_and_trafo_from_xml(xml_path, setup_id, t_start,
    106                                                 name, menu_name, trafos_for_mobie)
    108 tmp_folder_ = None if tmp_folder is None else f"{tmp_folder}_{name}"
--> 109 add_image(data_path, input_key, root, dataset_name,
    110           image_name=name, resolution=resolution, scale_factors=scale_factors,
    111           chunks=chunks, file_format=file_format, menu_name=menu_name,
    112           tmp_folder=tmp_folder_, target=target, max_jobs=max_jobs,
    113           unit=unit, view=view, transformation=transformation,
    114           is_default_dataset=is_default_dataset, description=description)

File /g/emcf/schorb/code/mobie-utils-python/mobie/image_data.py:155, in add_image(input_path, input_key, root, dataset_name, image_name, resolution, scale_factors, chunks, file_format, menu_name, tmp_folder, target, max_jobs, view, transformation, unit, is_default_dataset, description)
    118 def add_image(input_path, input_key,
    119               root, dataset_name, image_name,
    120               resolution, scale_factors, chunks,
   (...)
    126               is_default_dataset=False,
    127               description=None):
    128     """ Add an image source to a MoBIE dataset.
    129 
    130     Will create the dataset if it does not exist.
   (...)
    153         description [str] - description for this image (default: None)
    154     """
--> 155     view = utils.require_dataset_and_view(root, dataset_name, file_format,
    156                                           source_type="image", source_name=image_name,
    157                                           menu_name=menu_name, view=view,
    158                                           is_default_dataset=is_default_dataset)
    160     dataset_folder = os.path.join(root, dataset_name)
    161     tmp_folder = f'tmp_{dataset_name}_{image_name}' if tmp_folder is None else tmp_folder

File /g/emcf/schorb/code/mobie-utils-python/mobie/utils.py:66, in require_dataset_and_view(root, dataset_name, file_format, source_type, source_name, menu_name, view, is_default_dataset)
     64 elif view is not None and menu_name is not None:
     65     view.update({"uiSelectionGroup": menu_name})
---> 66 validate_view_metadata(view, sources=[source_name])
     68 if not ds_exists:
     69     metadata.create_dataset_structure(root, dataset_name, [file_format])

File /g/emcf/schorb/code/mobie-utils-python/mobie/validation/metadata.py:128, in validate_view_metadata(view, sources, dataset_folder, assert_true)
    126 except ValidationError as e:
    127     msg = f"{e}"
--> 128     assert_true(False, msg)
    130 displays = view.get("sourceDisplays")
    131 # dynamic validation of valid sources if sources argument is passed

File /g/emcf/schorb/code/mobie-utils-python/mobie/validation/utils.py:64, in _assert_true(expr, msg)
     62 def _assert_true(expr, msg=""):
     63     if not expr:
---> 64         raise ValueError(msg)

ValueError: None is not of type 'string'

Failed validating 'type' in schema['properties']['uiSelectionGroup']:
    {'pattern': '^[^;\\/ ]+$', 'type': 'string'}

On instance['uiSelectionGroup']:
    None

Is there anything else I need to provide? Or what would be a clean way to quickly populate a MoBIE project from existing n5 data?

I thought that this would maybe a way to skip manually managing the datasets and sources.

constantinpape commented 2 years ago

There was still a bug in the code that made add_bdv_image fail when not passing an argument for menu_name. I have fixed this now, but I would suggest to still pass menu_name to the function; otherwise it will just default to "images", which is pretty non-descriptive.

martinschorb commented 2 years ago

OK, it seems to run now. 👍 Is there an option to make add_bdv_image skip the copy process if the input is already a MoBIE-compatible container (N5/Zarr)? I would envision that the function will behave as a wrapper for the metadata and directory structure creation. As @K-Meech 's project creator it would either link or move existing BDV images into the project.

constantinpape commented 2 years ago

Is there an option to make add_bdv_image skip the copy process if the input is already a MoBIE-compatible container (N5/Zarr)? I would envision that the function will behave as a wrapper for the metadata and directory structure creation. As @K-Meech 's project creator it would either link or move existing BDV images into the project

No, that's not supported by add_bdv_image and should go into a separate function. In general I don't really like the linking approaches as it will make publishing a MoBIE project not possible; everything should be contained in the MoBIE directory so that it can be uploaded to s3.

Moving the image data is of course fine, but I am a bit hesitant, because it changes the input data to the function, which users might not expect. Implementing functionality for this would be simple though: You can base it on add_bdv_image (which calls add_image internally), but instead of calling import_image_data move the data.

martinschorb commented 2 years ago

I will have a look.

martinschorb commented 2 years ago

I found some inconsistency: It seems that import_image_data and the DownscalingWorkflow it calls also creates some kind of basic metadata. metadata_dict which seems MoBIE-specific. I will work around it and generate this partially populated dataset template separately. But maybe it makes sense to pull it out of there and include it into this MoBIE codebase.

constantinpape commented 2 years ago

It seems that import_image_data and the DownscalingWorkflow it calls also creates some kind of basic metadata. metadata_dict which seems MoBIE-specific.

No, this is not MoBIE specific at all but just the metadata that will be used to create the bdv xml file (more precisely to create the transformation and resolution metadata in there). (Or alternatively it will be used to generate the multiscales metadata for ome.zarr) .You don't need this if you move the bdv xml file as well as the n5 file.

martinschorb commented 2 years ago

OK, sorry. I was mislead by an error message complaining about a missing source entry in the dataset. And thus I thought that this was created during th downsampling. But in fact it happens before...

martinschorb commented 2 years ago

What's the rationale behind requiring the same source and the internal ViewSetup name (in the XML)? Is it the BDV source names that it retrieves when opening an XML that need to be consistent with the source name in the view?

I have trouble with the N5 and XML automatically being renamed to "Setup0" because of that and then obviously the ImageLoader part in the XML no longer works. So I indeed have to modify the XML in any case.

constantinpape commented 2 years ago

Is it the BDV source names that it retrieves when opening an XML that need to be consistent with the source name in the view?

Yes, exactly.

So I indeed have to modify the XML in any case.

Yes, that will be necessary.

martinschorb commented 2 years ago

In the end it was rather easy. Pulling it out into other functions would have required massive code duplications. Hence I added it into add image

https://github.com/mobie/mobie-utils-python/pull/45

constantinpape commented 2 years ago

fixed by #45