constantinpape / cluster_tools

Distributed segmentation for bio-image-analysis
MIT License
34 stars 14 forks source link

wrong kwarg passed on to elf #34

Closed martinschorb closed 2 years ago

martinschorb commented 2 years ago

Hi,

when converting to MoBIE, I run into:

...
  File ".../cluster_tools/cluster_tools/copy_volume/copy_volume.py", line 138, in run_impl
    with vu.file_reader(self.output_path, mode="a", **file_kwargs) as f:
  File ".../cluster_tools/cluster_tools/utils/volume_utils.py", line 34, in file_reader
    return elf.io.open_file(path, mode=mode, **kwargs)
  File ".../elf/elf/io/files.py", line 45, in open_file
    return constructor(path, mode=mode, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'dimension_separator'

I have no idea where that dimension_separator comes from it seems to be not None otherwise https://github.com/constantinpape/cluster_tools/blob/4d25373bcad32b9eeee48bcfd5c65fa773a31e1c/cluster_tools/copy_volume/copy_volume.py#L137 should filter it. It does not appear in any of the luigi config files.

Also, like in https://github.com/mobie/mobie-utils-python/pull/57 I noticed the missing str() here: https://github.com/constantinpape/cluster_tools/blob/4d25373bcad32b9eeee48bcfd5c65fa773a31e1c/cluster_tools/copy_volume/copy_volume.py#L128

constantinpape commented 2 years ago

You are using an outdated version of z5py. Please update it then this will work.

Also, like in mobie/mobie-utils-python#57 I noticed the missing str() here:

Yes, that's a good point, can you make a PR?

martinschorb commented 2 years ago

You are using an outdated version of z5py

OK, I re-installed cluster_tools and mobie through pip from the git repos but that only checks for existence and does not update dependencies.

martinschorb commented 2 years ago

Yes, that's a good point, can you make a PR? https://github.com/constantinpape/cluster_tools/pull/35

martinschorb commented 2 years ago

Now with everythig updated I get:

...
...cluster_tools/copy_volume/copy_volume.py", line 162, in run_impl
    self.submit_jobs(n_jobs, self.prefix)
  File ".../cluster_tools/cluster_tasks.py", line 567, in submit_jobs
    tasks = [pp.submit(self._submit, job_id, job_prefix) for job_id in range(n_jobs)]
  File ".../cluster_tools/cluster_tasks.py", line 567, in <listcomp>
    tasks = [pp.submit(self._submit, job_id, job_prefix) for job_id in range(n_jobs)]
  File ".../python3.8/concurrent/futures/process.py", line 645, in submit
    self._start_queue_management_thread()
  File ".../python3.8/concurrent/futures/process.py", line 584, in _start_queue_management_thread
    self._adjust_process_count()
  File ".../python3.8/concurrent/futures/process.py", line 608, in _adjust_process_count
    p.start()
  File ".../python3.8/multiprocessing/process.py", line 118, in start
    assert not _current_process._config.get('daemon'), \
AssertionError: daemonic processes are not allowed to have children
constantinpape commented 2 years ago

OK, I re-installed cluster_tools and mobie through pip from the git repos but that only checks for existence and does not update dependencies.

Don't reinstall unrelated stuff through pip, but update z5py properly via conda:

conda install "z5py>=2.0.15"
constantinpape commented 2 years ago
...
...cluster_tools/copy_volume/copy_volume.py", line 162, in run_impl
    self.submit_jobs(n_jobs, self.prefix)
  File ".../cluster_tools/cluster_tasks.py", line 567, in submit_jobs
    tasks = [pp.submit(self._submit, job_id, job_prefix) for job_id in range(n_jobs)]
  File ".../cluster_tools/cluster_tasks.py", line 567, in <listcomp>
    tasks = [pp.submit(self._submit, job_id, job_prefix) for job_id in range(n_jobs)]
  File ".../python3.8/concurrent/futures/process.py", line 645, in submit
    self._start_queue_management_thread()
  File ".../python3.8/concurrent/futures/process.py", line 584, in _start_queue_management_thread
    self._adjust_process_count()
  File ".../python3.8/concurrent/futures/process.py", line 608, in _adjust_process_count
    p.start()
  File ".../python3.8/multiprocessing/process.py", line 118, in start
    assert not _current_process._config.get('daemon'), \
AssertionError: daemonic processes are not allowed to have children

I have no idea what this is, can you point me to the script you're using?

martinschorb commented 2 years ago

I am trying to re-initiate my conda env from the mobie environment file. Let's see if that fixes things. Otherwise the script is here:

https://github.com/mobie/centriole-tomo-examples/blob/main/join2bdv.py

constantinpape commented 2 years ago

Are you sure you have set target to slurm here: https://github.com/mobie/centriole-tomo-examples/blob/main/join2bdv.py#L19? Because the code-path where the error occurs should only be executed for target="local", and then also makes sense since you're trying to nest processes.

martinschorb commented 2 years ago

Yes, I was trying to debug the run locally. With slurm it runs.

martinschorb commented 2 years ago

waiting for the results ...

martinschorb commented 2 years ago

Huh, something weird is going on with the data. I looks like there is some chunk-wise normalization going on.

Screenshot 2022-04-14 at 13 49 42

I will convert again keeping the singed int type, but I am pretty sure it wasn't in there...

martinschorb commented 2 years ago

that's in /g/schwab/Tobias/MoBIE2

martinschorb commented 2 years ago

Yes, it clearly happens during the int_to_uint conversion.

Screenshot 2022-04-14 at 14 00 03

Compare the signed data in /g/schwab/Tobias/MoBIE_signed. (only showing the "black" zero values)

constantinpape commented 2 years ago

Can you please send me the exact filepaths to a file that has the issue and is converted to unsigned int and the corresponding file that is not converted?

martinschorb commented 2 years ago

/g/schwab/Tobias/MoBIE2/tomo/images/ome-zarr/MMRR_07_grid12_c004.ome.zarr vs. /g/schwab/Tobias/MoBIE_signed/tomo/images/ome-zarr/MMRR_07_grid12_c004.ome.zarr

constantinpape commented 2 years ago

Ok, the issue was that this runs into the wrong code path for conversion from float dtypes to uint8. And this conversion is not really correct in any case. The behaviour for int_to_uint will be fixed by #36. General fix for the uint8 conversion will come later; I will create an issue to keep track of this.

constantinpape commented 2 years ago

This issue is fixed now on master and I already made a new release, which should be on conda forge after Easter. And I made a follow-up issue for the incorrect uint8 conversion for float data: https://github.com/constantinpape/cluster_tools/issues/37