ome / omero-py

Python project containing Ice remoting code for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
21 stars 32 forks source link

Addding support for other encodings than utf-8 in DownloadingOriginalFileProvider #325

Open JulianHn opened 2 years ago

JulianHn commented 2 years ago

This pull request adds support for encodings differing from utf-8 in omero.utils.populate_roi.DownloadingOriginalFileProvider.get_original_file_data

This is relevant e.g. when using the Populate_Metadata.py script distributed with OMERO, when using a CSV File exported from Excel with default settings (raised as issue https://github.com/ome/omero-py/issues/323) An accompanying pull request to omero-scripts that adds support for this new functionality will be made as well (#198).

I could not find a test for the get_original_file_data or anything from this module in /test, did I miss something?

// Julian

JulianHn commented 2 years ago

I've converted this to draft status until discussion in #198 over in omero-scripts is finished and possibly necessary changes to the get_original_file_data function are implemeted here.

joshmoore commented 2 years ago

Launched tests.

JulianHn commented 2 years ago

I've added a try-catch in the get_original_file_data() function that adds a message to the UnicodeDecodeError and then reraises the error to make this a bit easier to diagnose for a non-experienced user.

will-moore commented 2 years ago

When using this code from a script (reading a csv file) I got this as my entire sterr:

Traceback (most recent call last):
  File "./script", line 198, in <module>
    run_script()
  File "./script", line 190, in run_script
    message = populate_metadata(client, conn, script_params)
  File "./script", line 123, in populate_metadata
    data_for_preprocessing = provider.get_original_file_data(original_file)
  File "/home/omero/workspace/OMERO-server/.venv3/lib64/python3.6/site-packages/omero/util/populate_roi.py", line 194, in get_original_file_data
    temporary_file.seek(0)
  File "/usr/lib64/python3.6/tempfile.py", line 485, in func_wrapper
    return func(*args, **kwargs)
ValueError: I/O operation on closed file.

I guess the try/finally closed the file handle, then seek() raised an exception.

The previous code didn't close the file, so not sure you need to do that (except maybe if you actually catch an exception)?

JulianHn commented 2 years ago

@will-moore Woah, thanks for catching that. Not sure why I did not catch this when running my own small tests, I need to check my test setup apparently. I'm pretty sure I meant to put that in the except part, not in a finally part. I'll check and modify accordingly.

will-moore commented 2 years ago

@JulianHn I removed this from our daily build (to allow testing/merge of https://github.com/ome/omero-scripts/pull/195, which was holding up your other PR at https://github.com/ome/omero-scripts/pull/198)! Let us know when you've had a look and want to try including it again. Cheers

JulianHn commented 2 years ago

@will-moore Thanks for the info will let you know. That I did not catch this myself has made me suspicious of my testing, so I want to double check things again.