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

Numpy imports 10217 #20

Closed
wants to merge 9 commits into from
Closed

Conversation

will-moore
Copy link
Member

Add try/catch on numpy imports in various scripts.
If numpy is not installed, when you try to get params for a script, you see no parameters returned and a message indicating that numpy is required.

If you still try to run the script, you get a message telling you that numpy was required.

Don't know a nice way to simulate import failure on test server. Locally I just removed the /numpy folder from my filesystem.

Screen shot 2013-03-14 at 16 10 08

Screen shot 2013-03-14 at 15 22 55


dataTypes = [rstring('Image')]
sumAvgOptions = [rstring('Average'), rstring('Sum'), rstring('Average, with raw data')]

client = scripts.client('Plot_Profile.py', """This script processes Images, which have Line or PolyLine ROIs and outputs
the data as csv files, for plotting in E.g. exell.""",
the pixel intensity as csv files, for plotting in E.g. Excell.""",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Excel"? (-: (while you're here anyway)

@joshmoore
Copy link
Member

Two issues with this Will:

  • For our own scripts, it's certainly possible to add the numpy warning, but it's unlikely that others will do the same. Would it be possible to add a helper like IceImport that others could use themselves?
  • More importantly, though, the second definition of scripts.client may be corrupting things for users. I would think only the first invocation of params() is cached in the server. After you re-install numpy, the bin/omero scripts params would show no params, I would think. I can check this locally if you want.

@will-moore
Copy link
Member Author

Hmm - OK, so maybe I should only check my numpyImported flag when running the script and return the failure message (as I'm doing now) but not attempt to change the parameters returned.

That may make more sense then, since you can see more details of the script without numpy installed and make a more informed decision about whether you really need to.

This will still fix the original issue in the ticket, which was a failure to catch the ImportException, so you couldn't even see the script UI etc.

@will-moore
Copy link
Member Author

With those last commits, we now only handle import failure (of numpy or PIL) when the script runs. No difference in getting params etc. We also avoid importing util.imageUtil and util.figureUtil unless the PIL import is OK (since they will fail to import if PIL isn't available - see https://trac.openmicroscopy.org.uk/ome/ticket/10217#comment:4).

@joshmoore
Copy link
Member

@will-moore: I wonder if it would be easier/more appropriate to try to parse the files externally to determine these issues? Something like pylint which runs before script execution and throws a differente exception? My worry is that these changes are just very invasive and not really something we can or should expect others to replicate.

/cc @chris-allan, @manics

@manics
Copy link
Member

manics commented Mar 18, 2013

Tested a couple of scripts with and without numpy in web and insight, works as expected.

The docs already mention that numpy is required, though providing more information to a user is always good. Run-time feels like the wrong place though- isn't this something that a sysadmin should be dealing with at installation (either installing numpy, or removing scripts)?

@will-moore
Copy link
Member Author

We have always stated that you need to install PIL (less so with numpy), but we still get people who forget etc. Or don't think they're gonna need it, until it's too late. Hence the need for some run-time solution. I agree that it's not the nicest solution, and to have to copy & paste into every script, so if there's a better way....?

@joshmoore
Copy link
Member

Picking up discussion again with @will-moore back from VIZBI: and if we put logic in processor.py to scan the file for certain import statements (e.g. import\s.*numpy || numpy.*\simport). Or even more formally, something like http://people.debian.org/~morph/import_checker.py from http://sandrotosi.blogspot.de/2008/11/parse-python-files-for-import.html:

$ python p.py ~/git/components/tools/OmeroPy/scripts/omero/util_scripts/Combine_Images.py
Import stmt: numpy
Import stmt: re
From stmt: numpy
Import stmt: omero
Import stmt: omero.scripts
From stmt: omero.gateway
Import stmt: omero.constants
From stmt: omero.rtypes
Import stmt: omero.util.script_utils
Import stmt: time

@manics
Copy link
Member

manics commented Mar 25, 2013

Or even more formally, something like http://people.debian.org/~morph/import_checker.py

It claims to need python 2.5, though seem to work on 2.7. Handling

try:
    from PIL import Image, ImageDraw # see ticket:2597
except ImportError:
    import Image, ImageDraw # see ticket:2597

could be more tricky though.

@jburel
Copy link
Member

jburel commented Mar 27, 2013

2.4 is still the minimum requirement

@will-moore
Copy link
Member Author

@jburel: Discussed with @joshmoore - Lets see if we can simply handle the exception thrown by scriptService.getParams(), parsing it for import errors and then display something to the user. Will have to be done in web, then Insight.

@will-moore will-moore closed this Mar 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants