A suggestion for a more robust and modular import system

[smathot]

Right now, expyriment always imports everything. This is not only uneccesary, but also problematic for two reasons:

1. It eats up resources; and
2. It's fragile: If one thing breaks, everything breaks. I experienced this today when trying to run the Python 3 branch. The stimuli._video and io._parallel_port modules were broken, and--even though I didn't even use those--this prevented me from using expyriment alltogether.

I've been thinking about the best way to improve this without changing the import scheme from the perspective of the user. For example, expyriment.stimui.__init__.py has this line:

from ._canvas import Canvas

This allows users to do:

from expyriment.stimuli import Canvas
my_canvas = Canvas()

This way of importing is convenient for users, but the implementation is suboptimal, for the two reasons I described above. One way to improve this would be to change expyriment.stimui.__init__.py so that it contains:

def Canvas(*arglist, **kwdict):
  from ._canvas import Canvas as RealCanvas
  return RealCanvas(*arglist, **kwdict)

In other words, instead of directly importing the Canvas class from _canvas, you define a factory function that only imports when necessary. This works identical in every way, except:

1. If you check properties of the (uninstantiated) Canvas class itself, because it is now a function; and
2. If you check the docstrings of an (uninstantiated) Canvas. So this is actually a side-effect of 1. Docstrings cannot be copied without importing _canvas, thus defeating the purpose of the factory function.

Point 2 may be problematic in some cases, for example for users who use an editor that automatically shows these docstrings. So you could even imagine making this optional. For example, say that there is an environment variable called EXPYRIMENT_LAZY_IMPORT, then you could do:

import os

if 'EXPYRIMENT_LAZY_IMPORT' in os.environ and os.environ['EXPYRIMENT_LAZY_IMPORT'] == '1':
  def Canvas(*arglist, **kwdict):
    from ._canvas import Canvas as RealCanvas
    return RealCanvas(*arglist, **kwdict)
else:
  from ._canvas import Canvas

What do you think?

[lindemann09]

Yes, I'm also not very happy with fact that importing all classes causes
Expyriment to be unnecessarily fragile. We made this design decision
deliberately, because it is highly convenient, especially if you use a
editor with display the structure of modules while typing. It is a kind
of self-documentation.

I think you idea is very nice and a something like a good compromise. We
will discuss it and think abut it carefully. I like it. It doesn't seem
to be hard to implement.

Best,
Oliver

On 31.07.2016 16:47, Sebastiaan Mathot wrote:

Right now, expyriment always imports everything. This is not only
uneccesary, but also problematic for two reasons:

1. It eats up resources; and
2. It's fragile: If one thing breaks, everything breaks. I
   experienced this today when trying to run the Python 3 branch. The
   |stimuli._video| and |io._parallel_port| modules were broken,
   and--even though I didn't even use those--this prevented me from
   using expyriment alltogether.

I've been thinking about the best way to improve this without changing
the import scheme from the perspective of the user. For example,
|expyriment.stimui.init.py| has this line:

from ._canvasimport Canvas

This allows users to do:

from expyriment.stimuliimport Canvas
my_canvas= Canvas()

This way of importing is convenient for users, but the implementation
is suboptimal, for the two reasons I described above. One way to
improve this would be to change |expyriment.stimui.init.py| so
that it contains:

def Canvas(_arglist,__kwdict):
from ._canvasimport Canvasas RealCanvas
return RealCanvas(_arglist,**kwdict)

In other words, instead of directly importing the |Canvas| class from
|_canvas|, you define a factory function that only imports when
necessary. This works identical in every way, except:

1. If you check properties of the (uninstantiated) |Canvas| class
   itself, because it is now a function; and
2. If you check the docstrings of an (uninstantiated) |Canvas|. So
   this is actually a side-effect of 1. Docstrings cannot be copied
   without importing |_canvas|, thus defeating the purpose of the
   factory function.

Point 2 may be problematic in some cases, for example for users who
use an editor that automatically shows these docstrings. So you could
even imagine making this optional. For example, say that there is an
environment variable called |EXPYRIMENT_LAZY_IMPORT|, then you could do:

import os

if 'EXPYRIMENT_LAZY_IMPORT' in os.environand os.environ['EXPYRIMENT_LAZY_IMPORT']== '1':
def Canvas(_arglist,__kwdict):
from ._canvasimport Canvasas RealCanvas
return RealCanvas(_arglist,**kwdict)
else:
from ._canvasimport Canvas

What do you think?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#88, or mute the
thread
https://github.com/notifications/unsubscribe-auth/ACGc5h6qQKbgbLm_6-QiwJ0rNhgZQXPMks5qbLWPgaJpZM4JZC8a.

Dr. Oliver Lindemann
Division of Cognitive Science - University of Potsdam
http://www.cognitive-psychology.eu/lindemann

[smathot]

Yet another solution would be to add the docstrings to the factory function (and not to the class, to avoid duplication), and copy them to the instantiated objects at runtime. Like so:

def Canvas(*arglist, **kwdict):

    """Create a canvas.
    Parameters
    ----------
    size : (int, int)
        size of the canvas (int,int)
    position : (int, int), optional
        position of the stimulus
    colour : (int,int,int), optional
        colour of the canvas stimulus
    """

    from ._canvas import Canvas as RealCanvas
    c = RealCanvas(*arglist, **kwdict)
    c.__doc__ = Canvas.__doc__
    return c

Edit: This actually doesn't make sense, as it copies the docstring of the constructor to the object itself. Copying is probably not even really necessary, or, if implemented, it should be c.__init__.__doc__ = Canvas.__doc__.

[lindemann09]

Yes that one option. But I would do it only for some critical classes
that might not work.

Definitively, you are right, video and parallel port should not case
Expyriment to crash!

As an alternative solution, we could still import everything and
through an exception for critical modules while instantiation, if
something with the import went wrong. We are doing this at the moment
for the serial port: see
https://github.com/expyriment/expyriment/blob/python3/expyriment/io/_serialport.py#L88

Oliver

On 01.08.2016 09:44, Sebastiaan Mathot wrote:

Yet another solution would be to add the docstrings to the factory
function (and not to the class, to avoid duplication), and copy them
to the instantiated objects at runtime. Like so:

def Canvas(arglist,*kwdict):

 """Create a canvas.

Parameters

size : (int, int)
size of the canvas (int,int)
position : (int, int), optional
position of the stimulus
colour : (int,int,int), optional
colour of the canvas stimulus
"""

 from  ._canvasimport  Canvasas  RealCanvas
 c=  RealCanvas(*arglist,**kwdict)
 c.__doc__  =  Canvas.__doc__
 return  c

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#88 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACGc5vsm0x3Tn3OQhx-2A1PWneLk1FQGks5qbaPKgaJpZM4JZC8a.

Dr. Oliver Lindemann
Division of Cognitive Science - University of Potsdam
http://www.cognitive-psychology.eu/lindemann

[fladd]

Hi Sebastiaan,

Thanks for your suggestion. Oliver and me discussed this in the past actually. Back then we decided to do this for the extras, but not for the core modules. Since 0.8.0 the extras have hence to be imported manually, in addition to Expyriment itself. To me, this already was a compromise as it means that the extras will not be visible any longer to someone who uses Expyriment interactively. Since the extras are, well, extra features, their non-visibility is somehow justifiable. For the rest of the Expyriment, however, I don't think it is. To me, proper introspection is absolute key for a library.

Your suggestion, while being a great one technically, unfortunately will also break proper introspection, as you have mentioned yourself. I therefore am with Oliver on this, and I think we should rather catch critical imports and show a warning if it goes wrong (this was actually the intention for _parallel, but it obviously does not work properly yet).

For your two specific cases the fixes seem easy. In _parallel, I will just wrap the whole import in a try block. I am still a bit puzzled why this crashes, though. Did you install the 64bit or 32bit inpout32.dll? I am asking, because on several other machines (also Windows), I do not see this behaviour. However, I am using 32bit Python and 32bit inpout32.dll on all of those, so it might be specific to 64bit.
The bug in Video, well, that just resulted from me pushing something without testing it properly (as I pushed it to a development branch, not master). How tabs could sneak into this code is puzzling so...as I never use tabs :-)

So to sum up, I think we should of course just make sure that for a release everything in core works and Expyriment does not crash when imported. To achieve this, we will create alpha/beta releases for you and others to test, once we have implemented all the features for the next release. Until then, the development branches might be broken from time to time, while we work on these features.

[lindemann09]

@florian, your try block in _parallel.py wasn't catching all
exception. I fixed it!

Oliver

On 01.08.2016 10:23, Florian Krause wrote:

Hi Sebastiaan,

Thanks for your suggestion. Oliver and me discussed this in the past
actually. Back then we decided to do this for the extras, but not for
the core modules. Since 0.8.0 the extras have hence to be imported
manually, in addition to Expyriment itself. To me, this already was a
compromise as it means that the extras will not be visible any longer
to someone who uses Expyriment interactively. Since the extras are,
well, extra features, their non-visibility is somehow justifiable. For
the rest of the Expyriment, however, I don't think it is. To me,
proper introspection is absolute key for a library.

Your suggestion, while being a great one technically, unfortunately
will also break proper introspection, as you have mentioned yourself.
I therefore am with Oliver on this, and I think we should rather catch
critical imports and show a warning if it goes wrong (this was
actually the intention for |_parallel|, but it obviously does not work
properly yet).

For your two specific cases the fixes seem easy. In |_parallel|, I
will just wrap the whole import in a try block. I am still a bit
puzzled why this crashes, though. Did you install the 64bit or 32bit
inpout32.dll? I am asking, because on several other machines (also
Windows), I do not see this behaviour. However, I am using 32bit
Python and 32bit inpout32.dll on all of those, so it might be specific
to 64bit.
The bug in Video, well, that just resulted from me pushing something
without testing it properly (as I pushed it to a development branch,
not master). How tabs could sneak into this code is puzzling so...as I
never use tabs :-)

So to sum up, I think we should of course just make sure that for a
release everything in core works and Expyriment does not crash when
imported. To achieve this, we will create alpha/beta releases for you
and others to test, once we have implemented all the features for the
next release. Until then, the development branches might be broken
from time to time, while we work on these features.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#88 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACGc5lkO7iMxtrymoG2JbHXw4STfLkJ5ks5qbaz6gaJpZM4JZC8a.

Dr. Oliver Lindemann
Division of Cognitive Science - University of Potsdam
http://www.cognitive-psychology.eu/lindemann

[smathot]

Your suggestion, while being a great one technically, unfortunately will also break proper introspection, as you have mentioned yourself.

Well, not if you can disable/ enable it through an environment variable. And the docstring-copying method also largely preserves introspection, at least at the level of docstrings.

Did you install the 64bit or 32bit inpout32.dll?

I haven't installed either. But that shouldn't be necessary, should it?

[fladd]

@lindemann09 Your fix would not check for dlportio if inpout32 failed. I fixed that now. @smathot could you check if this runs on your system?

[fladd]

@smathot Yes, the environment variable might be an option to think about. However, it would still break introspection if it is set to '1', as the wrapper is a function, and not a class.

[smathot]

However, it would still break introspection if it is set to '1', as the wrapper is a function, and not a class.

Right. What aspects of introspection should be preserved? My understanding is that it's basically all about the docstrings, right? So if the factory function has the same docstring as the constructor of the class that it produces, then it would be fully transparent docstring-wise.

Even more transparent is to use a type-morphing class that is essentially an undefined class that morphs into another class once it's instantiated. Believe it or not, the following works, and will give you a regular stimuli._canvas.Canvas object.

class Canvas(object):

    def __init__(self, *arglist, **kwdict):

        """Create a canvas.
        Parameters
        ----------
        size : (int, int)
            size of the canvas (int,int)
        position : (int, int), optional
            position of the stimulus
        colour : (int,int,int), optional
            colour of the canvas stimulus
        """

        from ._canvas import Canvas as RealCanvas
        self.__class__ = RealCanvas
        RealCanvas.__init__(self, *arglist, **kwdict)

[fladd]

Yes, that might be an option. We will look into this.

But help me again with what the benefits are that justify this overhead. I see mainly one advantage, and that is memory usage. However, on my machine the Python process uses 38.2MB memory after I imported expyriment. Considering that 13MB of this are Python itself, Pygame and OpenGL, that leaves 25MB for expyriment itself. It does not seem that much to me to be honest. Initializing a screen uses more memory even.

The downside I see is that we use the sanity check that all modules actually load properly. For instance, without this, you might have never stumbled upon the parallel port bug and we might not know of it (as it worked on all machines we tested).

[smathot]

But help me again with what the benefits are that justify this overhead. I see mainly one advantage, and that is memory usage. However, on my machine the Python process uses 38.2MB memory after I imported expyriment. Considering that 13MB of this are Python itself, Pygame and OpenGL, that leaves 25MB for expyriment itself. It does not seem that much to me to be honest. Initializing a screen uses more memory even.

Well, that's true, of course. It mostly just goes against my sense of elegance. And the fragility thing, speaking of which:

The downside I see is that we use the sanity check that all modules actually load properly. For instance, without this, you might have never stumbled upon the parallel port bug and we might not know of it (as it worked on all machines we tested).

But that would be better handled with an automated unittest. In OpenSesame I have unittests to check (among many other thing) the syntax (and in some cases importability) of all modules. But I don't import them all when OpenSesame starts!

[fladd]

I guess our sense of elegance differs here :-)
As mentioned before, our main objective of loading the whole library at once was transparency to the user. They should see what the library includes by introspection and self-documentation - something Python promotes. Loading the whole library (which, as demonstrated above, is not very heavy) is the most straight-forward and most transparent way to do that, and, most importantly, does not put any restrictions on the self-documentation (e.g. I can see what a Canvas can do, what methods it has, even before I create one). We will consider your suggestion of optional lazy-loading in OpenSesame for the future, though.

I agree with you that proper unit tests are very much needed, independent of the suggestion discussed here. I hope we will eventually have some time to implement them.

[fladd]

@smathot I will keep this in mind for the 1.0.0 release. For now, we would like to focus on getting the 0.9.0 (with Python 3 support) out quickly (expect a beta very soon).

[fladd]

Lazy loading mechanism

[lindemann09]

We are considering this now for the extras and maybe in future for all classes.