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

Support for Path objects in io packages #4606

Merged
merged 7 commits into from Mar 1, 2016

Conversation

Projects
None yet
3 participants
@anchitjain1234
Contributor

anchitjain1234 commented Feb 13, 2016

For #4412.
Right now added support for io.ascii by modifying get_readable_fileobj in utils because io.ascii uses get_readable_fileobj to read.
I would also try to add support for Path objects in other io packages.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 13, 2016

Contributor

@taldcroft Is this the correct approach? I tried to follow yours as in #4405.

Contributor

anchitjain1234 commented Feb 13, 2016

@taldcroft Is this the correct approach? I tried to follow yours as in #4405.

@anchitjain1234 anchitjain1234 changed the title from Added support for Path objects in get_readable_fileobj in utils to Support for Path objects in io packages Feb 13, 2016

Added support for Path objects in io.ascii.
By modifying get_readable_fileobj in utils because io.ascii uses get_readable_fileobj to read.
@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 15, 2016

Member

@anchitjain1234 - thanks, this looks promising! If you do a census throughout the project of places that use get_readable_fileobj, I would be nice to see demonstration through testing of this fix being more broadly applicable than just io.ascii. I'm not sure off-hand how io.fits and io.votable handle the front-end task of handling the "input" file, which might be one of many different object types.

Member

taldcroft commented Feb 15, 2016

@anchitjain1234 - thanks, this looks promising! If you do a census throughout the project of places that use get_readable_fileobj, I would be nice to see demonstration through testing of this fix being more broadly applicable than just io.ascii. I'm not sure off-hand how io.fits and io.votable handle the front-end task of handling the "input" file, which might be one of many different object types.

@taldcroft taldcroft self-assigned this Feb 15, 2016

@taldcroft taldcroft added this to the v1.2.0 milestone Feb 15, 2016

anchitjain1234 added some commits Feb 18, 2016

Added test for votable.validate when source is Path object.
Also modified docstrings of io.votable.validate and io.ascii.read for indicating capability to read Path objects
Added handling of pathlib.Path object in io.fits .
Also modified docstring of fitsopen in hdulist.py
@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 18, 2016

Contributor

If you do a census throughout the project of places that use get_readable_fileobj, I would be nice to see demonstration through testing of this fix being more broadly applicable than just io.ascii.

@taldcroft I grepped the whole project to find the usage of get_readable_fileobj. Check results here.
From the above results, I think I have covered through tests io.ascii, io.votable and utils.
But I am not sure if vo also needs to be tested? because it uses get_readable_fileobj to get data from URL only

I'm not sure off-hand how io.fits and io.votable handle the front-end task of handling the "input" file

I handled io.fits directly in _File class and io.votable is automatically accepting pathlib.Path object because it uses utils.xml.iterparser which in turn uses get_readable_fileobj.

I also added a separate test for votable.validate because it uses get_readable_fileobj directly to get content.

Contributor

anchitjain1234 commented Feb 18, 2016

If you do a census throughout the project of places that use get_readable_fileobj, I would be nice to see demonstration through testing of this fix being more broadly applicable than just io.ascii.

@taldcroft I grepped the whole project to find the usage of get_readable_fileobj. Check results here.
From the above results, I think I have covered through tests io.ascii, io.votable and utils.
But I am not sure if vo also needs to be tested? because it uses get_readable_fileobj to get data from URL only

I'm not sure off-hand how io.fits and io.votable handle the front-end task of handling the "input" file

I handled io.fits directly in _File class and io.votable is automatically accepting pathlib.Path object because it uses utils.xml.iterparser which in turn uses get_readable_fileobj.

I also added a separate test for votable.validate because it uses get_readable_fileobj directly to get content.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 23, 2016

Contributor

@taldcroft Could you please check the changes?

Contributor

anchitjain1234 commented Feb 23, 2016

@taldcroft Could you please check the changes?

Show outdated Hide outdated astropy/io/fits/file.py
@@ -97,6 +104,9 @@ def __init__(self, fileobj=None, mode=None, memmap=None, clobber=False,
return
else:
self.simulateonly = False
# If fileobj is of type pathlib.Path
if PY3 and HAS_PATHLIB and isinstance(fileobj, Path):

This comment has been minimized.

@taldcroft

taldcroft Feb 23, 2016

Member

You don't need to check PY3, all that matters is HAS_PATHLIB to make sure the second logical test will execute properly.

@taldcroft

taldcroft Feb 23, 2016

Member

You don't need to check PY3, all that matters is HAS_PATHLIB to make sure the second logical test will execute properly.

Show outdated Hide outdated astropy/io/fits/file.py
@@ -74,6 +74,13 @@
PKZIP_MAGIC = b('\x50\x4b\x03\x04')
BZIP2_MAGIC = b('\x42\x5a')
try:
from pathlib import Path

This comment has been minimized.

@taldcroft

taldcroft Feb 23, 2016

Member

Make this import pathlib for consistency with usage in the other modules.

@taldcroft

taldcroft Feb 23, 2016

Member

Make this import pathlib for consistency with usage in the other modules.

Show outdated Hide outdated astropy/io/votable/tests/vo_test.py
def test_validate_path_object():
"""
Validating when source is passed as path object. (#4412)
Creating different method from ``test_validate`` so that it could be

This comment has been minimized.

@taldcroft

taldcroft Feb 23, 2016

Member

I think you can just add an argument to test_validate, e.g.

def test_validate(test_path_object=False):
    fpath = get_pkg_data_filename('data/regression.xml')
    if test_path_object:
        fpath = pathlib.Path(fpath)
    ...

Then you can still have a separate test_validate_path_object() (which will be skipped for PY2) but just calls test_validate(test_path_object=True).

@taldcroft

taldcroft Feb 23, 2016

Member

I think you can just add an argument to test_validate, e.g.

def test_validate(test_path_object=False):
    fpath = get_pkg_data_filename('data/regression.xml')
    if test_path_object:
        fpath = pathlib.Path(fpath)
    ...

Then you can still have a separate test_validate_path_object() (which will be skipped for PY2) but just calls test_validate(test_path_object=True).

Show outdated Hide outdated astropy/utils/data.py
@@ -167,6 +167,14 @@ def get_readable_fileobj(name_or_obj, encoding=None, cache=False,
# passed in. In that case it is not the responsibility of this
# function to close it: doing so could result in a "double close"
# and an "invalid file descriptor" exception.
PATH_TYPES = six.string_types
try:
from pathlib import Path

This comment has been minimized.

@taldcroft

taldcroft Feb 23, 2016

Member

Better to follow the same idiom from testing and define HAS_PATHLIB. This allows for the possibility of a Python 4 that may or may not have pathlib.

Note: you should change my original implementation in io.registry to match (#4405). I fell into the mistake of testing for PY3. See http://astrofrog.github.io/blog/2016/01/12/stop-writing-python-4-incompatible-code/. In fact the fix to io.registry might not be needed now that you are patching get_readable_fileobj. I haven't looked through the logic that closely but you should look and test.

@taldcroft

taldcroft Feb 23, 2016

Member

Better to follow the same idiom from testing and define HAS_PATHLIB. This allows for the possibility of a Python 4 that may or may not have pathlib.

Note: you should change my original implementation in io.registry to match (#4405). I fell into the mistake of testing for PY3. See http://astrofrog.github.io/blog/2016/01/12/stop-writing-python-4-incompatible-code/. In fact the fix to io.registry might not be needed now that you are patching get_readable_fileobj. I haven't looked through the logic that closely but you should look and test.

Show outdated Hide outdated astropy/utils/data.py
if isinstance(name_or_obj, six.string_types):
if isinstance(name_or_obj, PATH_TYPES):
# name_or_obj could be a Path object in Python 3
if six.PY3:

This comment has been minimized.

@taldcroft

taldcroft Feb 23, 2016

Member

Notice that this code becomes easier to understand:

        if HAS_PATHLIB:
            ...

This avoids one level of contextual knowledge that only Python 3 has pathlib. So then the comment would be more like # name_or_obj could be a Path object if pathlib is available.

@taldcroft

taldcroft Feb 23, 2016

Member

Notice that this code becomes easier to understand:

        if HAS_PATHLIB:
            ...

This avoids one level of contextual knowledge that only Python 3 has pathlib. So then the comment would be more like # name_or_obj could be a Path object if pathlib is available.

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 23, 2016

Member

@anchitjain1234 - looks pretty good, thanks!

Member

taldcroft commented Feb 23, 2016

@anchitjain1234 - looks pretty good, thanks!

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Feb 23, 2016

Contributor

Also pathlib can be installed on PY2: https://pypi.python.org/pypi/pathlib2/
So it is better to avoid testing for PY3 as @taldcroft said.

Contributor

saimn commented Feb 23, 2016

Also pathlib can be installed on PY2: https://pypi.python.org/pypi/pathlib2/
So it is better to avoid testing for PY3 as @taldcroft said.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 25, 2016

Contributor

@taldcroft Please check the changes.

Contributor

anchitjain1234 commented Feb 25, 2016

@taldcroft Please check the changes.

Show outdated Hide outdated astropy/io/registry.py
if six.PY3:
# path might be a pathlib.Path object if HAS_PATHLIB,
# so coerce to a regular string.
if HAS_PATHLIB:

This comment has been minimized.

@taldcroft

taldcroft Feb 25, 2016

Member

It occurs to me that in theory a user might be passing a unicode file name on Python 2 with the backport of pathlib independently installed. Then conversion with str() would be a problem. So maybe you need to also check isinstance(args[0], pathlib.Path)?

@taldcroft

taldcroft Feb 25, 2016

Member

It occurs to me that in theory a user might be passing a unicode file name on Python 2 with the backport of pathlib independently installed. Then conversion with str() would be a problem. So maybe you need to also check isinstance(args[0], pathlib.Path)?

This comment has been minimized.

@anchitjain1234

anchitjain1234 Feb 28, 2016

Contributor

@taldcroft If I understand correctly, you meant to say for example a file named x©.fits on Python 2 with the backport of pathlib independently installed would be a problem?
I cant understand why would it be a problem?
I also tested it on my system on Python 2 and Python 3 where it works correctly.

@anchitjain1234

anchitjain1234 Feb 28, 2016

Contributor

@taldcroft If I understand correctly, you meant to say for example a file named x©.fits on Python 2 with the backport of pathlib independently installed would be a problem?
I cant understand why would it be a problem?
I also tested it on my system on Python 2 and Python 3 where it works correctly.

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 28, 2016

Member

@anchitjain1234 - looks good! Now you just need to add an entry to CHANGES.rst describing this update. You can put [skip ci] in the commit message to avoid re-running CI testing.

Member

taldcroft commented Feb 28, 2016

@anchitjain1234 - looks good! Now you just need to add an entry to CHANGES.rst describing this update. You can put [skip ci] in the commit message to avoid re-running CI testing.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 29, 2016

Contributor

@taldcroft In Python 2 pathlib is pathlib 2 therefore import pathlib would not work. So do I need to add import pathlib2 statements also?

Contributor

anchitjain1234 commented Feb 29, 2016

@taldcroft In Python 2 pathlib is pathlib 2 therefore import pathlib would not work. So do I need to add import pathlib2 statements also?

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Feb 29, 2016

Contributor

@anchitjain1234 - Even if the package name is pathlib2, it will import as pathlib.

@taldcroft - Maybe the extra dependency should be added to a Python2 build in Travis ?

Contributor

saimn commented Feb 29, 2016

@anchitjain1234 - Even if the package name is pathlib2, it will import as pathlib.

@taldcroft - Maybe the extra dependency should be added to a Python2 build in Travis ?

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 29, 2016

Contributor

@taldcroft Changelog entry added.

Contributor

anchitjain1234 commented Feb 29, 2016

@taldcroft Changelog entry added.

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Feb 29, 2016

Member

@saimn - I think that @anchitjain1234 has a point. If pathlib2 is installed the code still needs to explicitly do:

import pathlib2 as pathlib

I was hoping that pip install pathlib2 did a magic rename so that import pathlib would work on Py2, but I just tried and that is not the case. So this leads to the uglyness:

try:
   import pathlib
except ImportError:
    try:
        import pathlib2 as pathlib
.. etc

I think we should just merge this current version, which addresses the stated issue of providing support for bona-fide pathlib.Path objects. Supporting the Python2 backport can be a separate issue, but I think it will need to go along with refactoring the HAS_PATHLIB logic into a separate single entity.

Member

taldcroft commented Feb 29, 2016

@saimn - I think that @anchitjain1234 has a point. If pathlib2 is installed the code still needs to explicitly do:

import pathlib2 as pathlib

I was hoping that pip install pathlib2 did a magic rename so that import pathlib would work on Py2, but I just tried and that is not the case. So this leads to the uglyness:

try:
   import pathlib
except ImportError:
    try:
        import pathlib2 as pathlib
.. etc

I think we should just merge this current version, which addresses the stated issue of providing support for bona-fide pathlib.Path objects. Supporting the Python2 backport can be a separate issue, but I think it will need to go along with refactoring the HAS_PATHLIB logic into a separate single entity.

@saimn

This comment has been minimized.

Show comment
Hide comment
@saimn

saimn Feb 29, 2016

Contributor

@taldcroft - Ah sorry, I tested this on a virtualenv, that I thought was based on PY2 but it seems not. The backport provides indeed a pathlib2 package, and this is intentional : mcmtroffaes/pathlib2#8 (Probably for good reasons even if it is not handy).

Contributor

saimn commented Feb 29, 2016

@taldcroft - Ah sorry, I tested this on a virtualenv, that I thought was based on PY2 but it seems not. The backport provides indeed a pathlib2 package, and this is intentional : mcmtroffaes/pathlib2#8 (Probably for good reasons even if it is not handy).

taldcroft added a commit that referenced this pull request Mar 1, 2016

Merge pull request #4606 from anchitjain1234/fix-4412
Support for Path objects in io packages

@taldcroft taldcroft merged commit a447a48 into astropy:master Mar 1, 2016

@taldcroft

This comment has been minimized.

Show comment
Hide comment
@taldcroft

taldcroft Mar 1, 2016

Member

Thanks @anchitjain1234 ! And @saimn for useful review and input.

Member

taldcroft commented Mar 1, 2016

Thanks @anchitjain1234 ! And @saimn for useful review and input.

@anchitjain1234 anchitjain1234 deleted the anchitjain1234:fix-4412 branch Apr 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment