Skip to content

Conversation

@JulianHn
Copy link

@JulianHn JulianHn commented Mar 28, 2022

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 #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

…FileProvider.get_original_file_data by passing an encoding argument
@JulianHn
Copy link
Author

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
Copy link
Member

Launched tests.

…original_file_data() to provide the user with more helpful error messages as well as enabling easy catching of the exception in calling scripts

Removed the custom error class again, since decoders that can fail decoding will always raise a UnicodeDecodeError according to
https://docs.python.org/3/library/codecs.html.
Modified try-catch to add a more specific note to the UnicodeError for better user information.
@JulianHn
Copy link
Author

JulianHn commented Jun 3, 2022

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.

@JulianHn JulianHn marked this pull request as ready for review June 3, 2022 14:18
@will-moore
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

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

@JulianHn
Copy link
Author

@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.

@sbesson
Copy link
Member

sbesson commented Mar 18, 2025

Closing as there has been no activity for the last years and this is conflicting with the HEAD of omero-py. Feel free to reopen if/when a new version is ready for retesting

@sbesson sbesson closed this Mar 18, 2025
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.

4 participants