Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#44308 closed defect (invalid)

py-SRPAstro: switch PIL/Pillow dependency to py-matplotlib and py-scipy.

Reported by: Ionic (Mihai Moldovan) Owned by: petrrr
Priority: Normal Milestone:
Component: ports Version:
Keywords: haspatch Cc: stefano.covino@…, seanfarley (Sean Farley), michaelld (Michael Dickens)
Port: py-SRPAstro py-matplotlib py-scipy

Description

py-SRPAstro has a (formal) dependency on PIL, although it doesn't use PIL in its code.

However, PIL is needed by both py-matplotlib and py-scipy -- dependencies of py-SRPAstro -- which currently, wrongly do not declare it as a dependency.

The author has confirmed in private mail, that not his software (py-SRPAstro) is requiring/using PIL, but indeed the other two ports.

Thus, switch over the PIL/Pillow dependency (in its path:-based form as suggested/worked on in #44285) from py-SRPAstro to py-matplotlib and py-scipy.

Attachments (1)

py-SRPAstro-py-matplotlib-py-scipy-pil-dep-move.patch (2.4 KB) - added by Ionic (Mihai Moldovan) 10 years ago.
Move PIL/Pillow dependency from py-SRPAstro to py-matplotlib and py-scipy. Revbump.

Download all attachments as: .zip

Change History (12)

Changed 10 years ago by Ionic (Mihai Moldovan)

Move PIL/Pillow dependency from py-SRPAstro to py-matplotlib and py-scipy. Revbump.

comment:1 Changed 10 years ago by Ionic (Mihai Moldovan)

Revbumped ports to account for dependency change.

comment:2 Changed 10 years ago by petrrr

openmainter should be added to py-SRPAstro, waiting for confirmation by the maintainer.

Last edited 10 years ago by petrrr (previous) (diff)

comment:3 Changed 10 years ago by petrrr

I just looked though the code. It specifies pil as an requirement, so I would argue that at least formally it depends on PIL. So this should be better addressed upstream.

sudo grep -i pil * -r 
[...]
SRPAstro.egg-info/requires.txt:pil
setup.py:    install_requires=['scipy', 'astlib>=0.4', 'pil', 'matplotlib', 'atpy',

comment:4 Changed 10 years ago by petrrr

I do not see why py-scipy should declare a dependency on PIL or Pillow, but I found the following:

scipy/misc/pilutil.py:Note that PIL is not a dependency of SciPy and this module is not
scipy/misc/pilutil.py:available on systems that don't have PIL installed.

If you still think it should, please document why!

comment:5 Changed 10 years ago by petrrr

For py-matplotlib there seems to be no strict requirement for PIL neither. Import are found only in lib/matplotlib/backend_bases.py, lib/matplotlib/image.py and lib/matplotlib/tests/test_image.py, but the absence of PIL is handled smoothly. All other occurrences are only in documentation or examples.

So the missing dependence on PIL seems to be intentional.

From lib/matplotlib/backend_bases.py:

try:
    from PIL import Image
    _has_pil = True
except ImportError:
    _has_pil = False

_backend_d = {}

From lib/matplotlib/image.py

    def pilread(fname):
        """try to load the image with PIL or return None"""
        try:
            from PIL import Image
        except ImportError:
            return None

From lib/matplotlib/tests/test_image.py:

try:
    from PIL import Image
    HAS_PIL = True
except ImportError:
    HAS_PIL = False

comment:6 Changed 10 years ago by petrrr

Cc: petr@… removed
Owner: changed from macports-tickets@… to petr@…
Status: newassigned

comment:7 Changed 10 years ago by petrrr

After reviewing this it looks like the dependencies of these ports are currently handled correctly.

The PIL dependency should stay with py-SRPAstro. If you still believe this port should not depend on PIL, please report upstream.

comment:8 Changed 10 years ago by petrrr

See ticket #44376, for change path:-based PIL/Pillow dependency, along with other dependency corrections.

comment:9 Changed 10 years ago by petrrr

Resolution: invalid
Status: assignedclosed

comment:10 Changed 10 years ago by Ionic (Mihai Moldovan)

scipy:

/opt/local/var/macports/build/_opt_macports_python_py-scipy/py26-scipy/work/scipy-0.14.0/scipy/misc/pilutil.py:    from PIL import Image, ImageFilter
/opt/local/var/macports/build/_opt_macports_python_py-scipy/py26-scipy/work/scipy-0.14.0/scipy/misc/pilutil.py:    import Image
/opt/local/var/macports/build/_opt_macports_python_py-scipy/py26-scipy/work/scipy-0.14.0/scipy/misc/pilutil.py:    import ImageFilter
/opt/local/var/macports/build/_opt_macports_python_py-scipy/py26-scipy/work/scipy-0.14.0/scipy/ndimage/io.py:        from PIL import Image

Yes, PIL is an optional dependency for both scipy and matplotlib, but I think it's highly recommended to have the modules around for having a complete feature set.

Maybe adding PIL/Pillow as a hard dependency is wrong, but it may certainly warrant a variant (to my mind even turned on by default.)

SRPAstro does formally depend on PIL, but only due to scipy/matplotlib. I haven't checked exactly, but I figure it uses classes from scipy/matplotlib which are only available with PIL. The author confirmed this.

comment:11 Changed 10 years ago by petrrr

I really believe the current approach is appropriate.

Intentionally neither matplotlib nor scipy declare a dependency on PIL, probably for some good reason (I could imagine to avoid any of the fuss with PIL), so MP should neither.

Adding a variant, does not provide any advantage, it just complicates the situation while installing exactly the same set of files. You cannot guaranty the right variant is installed anyway, but you can do this for PIL/Pillow. The extra functionally provided seems not to be expected by users or software (no tickets), and if you need it you just have to install PIL or Pillow, or just uninstall any time you want. So I see no problem here.

SRPAstro does exactly this, it expects this extra functionality provided by PIL, so it declares the dependency on it, and only this can guaranty it is around, with a variant you cannot be sure.

Anyway, if you disagree feel free to reopen, but in this case I would leave the decision to the maintainers of the ports.

Last edited 10 years ago by petrrr (previous) (diff)
Note: See TracTickets for help on using tickets.