[visionegg] Re: Obscure bug in visionegg

Dav Clark wrote:

Hi Andrew and the rest...
I've come across a strange bug in the ParameterTypes.py file in VisionEgg (1.1). The following code illustrates a crash: 

import VisionEgg
from VisionEgg.Text import Text
s = VisionEgg.Core.get_default_screen()
t = Text()
t.set(text='test')
If you look at the logic of ParameterTypes.get_type (line 326), you see that before testing if 'test' is a string, it's tested to see if it's a 4-item sequence, and then the string ends up as a Sequence4x4. A bit confusing, but it happens because substrings (like 'test'[0]) are also sequences, and so pass the relatively weak test on line 331 of ParameterTypes.py. It would be a lot easier to simply throw the thing at numpy.array and see if something sensible comes back.
So, in the code snippet above, t.set obtains that 'test' is a Sequence4x4 (which it's not!). Then, on line 387 of ParameterTypes.py, we catch an exception from assert_type each time through, which leads to control flowing eventually to line 397, where an undefined reference is made to require_class. So your program crashes with the rather nonintuitive UnboundLocalError. This would happen whenever an AnyOf type is not satisfied...
I guess you should raise a TypeError after the for loop ends (i.e. after line 391 of ParameterTypes.py, dedented two stops, similar to line 410)?
I could provide a patch, but the fix for assert_type is trivial, and otherwise I'm not exactly sure what the right thing to do is here... For example, I'm not sure any code expects to see these Sequence4x4 types. The only reference I can find to the type is in a comment on line 1054 of Core.PerspectiveProjection - the type doesn't actually seem to be used anywhere. More generally, it seems like there is type-related stuff left over that is now completely unused - probably a shift to using more standardized (python and numpy) types and shape attributes would be in order... I understand why the code developed it's own types, since types in python weren't great until at least python 2.3, which came out a year after VisionEgg. But now it seems the only useful one is "AnyOf".
Thanks for the report, Dav. I have fixed the 2 issues. For the 4 character string issue, I solved it by explicitly checking for string instances before checking for sequence instances. The UnboundLocal error was fixed along the lines you suggest. See http://visionegg.org/trac/changeset/1470
As you note, type-checking in VE pre-dates the decent types infrastructure introduced in Python (2.3?), but the reason for its existence is more fundamental. The basic idea is to protect programmers (actually, scientists without a lot of programming experience) from shooting themselves in the foot by explicitly raising errors when attempting to set variables to things that weren't going to work and would raise more obscure errors down the track. (As with many cases in Python when duck typing fails, PyOpenGL, to pick one example, doesn't always give clear errors when the input arguments are not what it's expecting.) If I were to redo the type-checking in any substantial way (a thought which fills me with dread, although not without merit), I would attempt to use enthought.traits, which also implements strict type checking. I would also drop the entire __slots__ usage in VE, which is an even worse example of defensive programming. These entire aspects of the VE comes from a time when I was torn in many directions about what it should be. A GUI application incorporating anything any experimenter would want to do? A single API incorporating the same? A programming library only for visual stimuli? As time passes, a programming library for visual stimuli is the niche I see being sustainable for VE. The high-level OpenGL wrapper is really the place where I think the VE can be the right tool for the job and the thing it does well. With that in mind, removing some or all of the defensive programming parts of VE would actually make sense, particularly since those are some of the trickier bits of code, as you have no doubt discovered. At least switching to enthought.traits would help greatly in that regard, although any major changes in that direction would be akin to open-heart surgery on the software. (And in the domain of extensive changes, I regard the idea of shifting to pyglet from PyOpenGL as higher priority due to easy multi-display possibilities.)
I note that Sequence4x4 is used for all instances of ProjectionBaseClass and its subclasses -- thus it is actually a fairly well used VE type. (See line 763 of Core.py.)
I'm also happy to submit a bug to your Trac, but there don't seem to be any real active tickets there... the only ticket being one that (I think) you already accomplished!
Hmm, it appears I can't even log into Trac to close that bug -- so I deleted it from the admin interface! I was having a lot of trouble with spam on the wiki and trac, so I've locked things down more than I'd like. With Trac it appears I locked them quite severely. Hmm, this will take some time to sort out, and I'd rather migrate VE hosting to something better anyway, so I'm just going to table this for now...
For now, the fix is simply not to use Parameter 'safe' code with string or "AnyOf" types, because it's actually quite dangerous!
OK, please let me know if this change works for you. (Is checking out from SVN OK, or should I make a tarball or zip file?)
This is a platform independent issue, but just for your reference, I'm running on OSX 10.5.4. with the system python (2.5.1). (Why am I running on the system python, you may ask? It's so I can use PyObjC 2 to control the bTop2 USB A/D box I've got). 

Let me know how I can help!
Dav
ps - I've written a wrapper around VisionEgg which I'd like to share - will anyone be at SciPy 2008? Might be a nice way to get some initial feedback.
Unfortunately, I'll be away from Caltech during SciPy '08, but I wish I could be there to talk with you... 
======================================
The Vision Egg mailing list
Archives: http://www.freelists.org/archives/visionegg
Website: http://www.visionegg.org/mailinglist.html

Other related posts: