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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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.

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.

3 participants