Add io.fits test showing that reading from URL works... - #6378

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

@astrofrog , I set "no change log required" but astropy-bot is still not satisfied ❓

@pllim - astro-bot picked up a new trigger. Probably the milestone change and the new label was added too quickly after each other for it to notice?

Would you be against to backport this to 2.0.x?

Thanks for the explanation, @bsipocz .

No, I am not against backporting if you don't mind the work. Feel free to re-milestone. 😃

@pllim - Doing a clean backport is super easy especially so close to the first release. If I run into non-trivial issues with the backport, then can always just remilesone it out of the bugfix milestone.

So feel free to mark the doc/test and obviously bug fixes whenever applicable.

This seems to work because you are using the file:// protocol (which is already a good thing), but it fails with a http url:

In [1]: import urllib.request
   ...: from astropy.io import fits

In [2]: obj = urllib.request.urlopen("http://data.muse-vlt.eu/HDFS/v1.0/EmiLines_HDFS_v1p0.fits")

In [3]: fits.open(obj)
---------------------------------------------------------------------------
UnsupportedOperation                      Traceback (most recent call last)
<ipython-input-3-e81a7a49e374> in <module>()
----> 1 fits.open(obj)

~/dev/astropy/astropy/io/fits/hdu/hdulist.py in fitsopen(name, mode, memmap, save_backup, cache, lazy_load_hdus, **kwargs)
    156 
    157     return HDUList.fromfile(name, mode, memmap, save_backup, cache,
--> 158                             lazy_load_hdus, **kwargs)
    159 
    160 

~/dev/astropy/astropy/io/fits/hdu/hdulist.py in fromfile(cls, fileobj, mode, memmap, save_backup, cache, lazy_load_hdus, **kwargs)
    409         return cls._readfrom(fileobj=fileobj, mode=mode, memmap=memmap,
    410                              save_backup=save_backup, cache=cache,
--> 411                              lazy_load_hdus=lazy_load_hdus, **kwargs)
    412 
    413     @classmethod

~/dev/astropy/astropy/io/fits/hdu/hdulist.py in _readfrom(cls, fileobj, data, mode, memmap, save_backup, cache, lazy_load_hdus, **kwargs)
    994             if not isinstance(fileobj, _File):
    995                 # instantiate a FITS file object (ffo)
--> 996                 fileobj = _File(fileobj, mode=mode, memmap=memmap, cache=cache)
    997             # The pyfits mode is determined by the _File initializer if the
    998             # supplied mode was None

~/dev/astropy/astropy/utils/decorators.py in wrapper(*args, **kwargs)
    505                         # one with the name of the new argument to the function
    506                         kwargs[new_name[i]] = value
--> 507             return function(*args, **kwargs)
    508 
    509         return wrapper

~/dev/astropy/astropy/io/fits/file.py in __init__(self, fileobj, mode, memmap, overwrite, cache)
    177             self.size = 0
    178         else:
--> 179             pos = self._file.tell()
    180             self._file.seek(0, 2)
    181             self.size = self._file.tell()

UnsupportedOperation: seek

@saimn Do you mean that this PR only partially adresses #4165?

@saimn, you're right. Is the URL that you provided one that would be reasonable to use in a unit test?

@MSeifert04 - I checked quickly as I'm trying to catch up with opened issues and PRs, but it looks like the issue is not closed. The current PR should also a test with a remote FITS file.

@drdavella - If you look at the Astropy tests, there is a mechanism for this kind of tests: with the @remote_data decorator, the get_pkg_data_filename function, and test files already available on a specific server for Astropy (https://astropy.stsci.edu/data/).

Also iirc, the io.fits _File class tries to read from file-like objects, i.e. with read, seek, tell attributes. But I don't know why the traceback I got is different from the one in #4165.

It seems like get_pkg_data_filename will download the file and provide a link to the cached version, but what I actually need is to use the real URL of the hosted file. Are there any other example tests that do this?

@drdavella - I suggest just use a url directly within a remote_data decorated test function.

Is it necessary to add a specific file to https://astropy.stsci.edu/data for this test, or can I just use one that already exists there?

it's great if you could reuse one from there, but can add another if it's necessary.

@saimn I think the method using BytesIO makes good sense and I updated the response handling logic to use that instead of download_files which, as you pointed out, is problematic. I left the request handling logic in place, although I agree that it seems to be of limited value.

In addition, I squashed everything a bit to make things look a little cleaner.

Thanks @drdavella , I changed the milestone to 3.0 hence we don't need to bother too much with Python 2 (we just need to keep the code compatible until Python 2 compatibility is removed). Can you move the changelog entry to 3.0 ? For Request objects, I would vote for removing this as I don't see how it can be useful, but it's up to you ;-)

@saimn okay I think I've addressed all the feedback:

• fixed import statement
• clarified change log comment and moved to 3.0.0
• removed support (and test) for HTTPRequest

ping @MSeifert04, if you want to have another look ?

I'm not sure if we actually should support open "response" objects. We already support reading from urls. Wouldn't it make sense to just error out in case a response is passed in with a helpful message (pass in the url instead)?

The question is to know if there is a real usecase for this. For HTTP responses, it is logical to support it because it is a file-like object, which is already supported by fits.open. And with this change, if you really want to use a Request object, you can use fits.open(urlopen(request)).

@saimn and @MSeifert04, it's easy enough for me to add back if you want. On the one hand, I agree with @saimn that there's probably not a real use case for this. On the other hand, I agree with @MSeifert04 that it's not that hard to support requests and it makes things seem more 'complete'. At this point I don't have strong feelings about it so I'll just wait for someone else to tell me what to do 😅.

@saimn But #6378 (comment) shows that while it's somewhat file-like it's not exactly file-like. So it would be "okay" not to support it.

I'm not sure either. It probably won't hurt to support HTTP responses but I'm a bit hesitant because it seems like we're trying to fix a non-existing problem.

@MSeifert04 :

But #6378 (comment) shows that while it's somewhat file-like it's not exactly file-like.

Yes, for HTTPResponse, hence the fix implemented currently. It allows to support the output of urlopen.

It probably won't hurt to support HTTP responses but I'm a bit hesitant because it seems like we're trying to fix a non-existing problem.

I'm confused by this, do you mean for responses, requests, or both ? For responses, as show above, it's almost file-like and I think it makes sense to support this. But for requests I'm also advocating that it's a non-existing problem.

I'm not sure what you mean by "requests". Personally I feel that passing an url to fits.open is a "request": fits.open("http://data.muse-vlt.eu/HDFS/v1.0/EmiLines_HDFS_v1p0.fits") (which works). Or did you mean the requests python package?

As I see it there is only one use-case to pass in responses: When the online material requires authentication, because that way you can't just pass in the url to fits.open. But for authentication it would be much safer and better to use the requests package (+HDUList.fromstring) or astroquery (if it's supported there).

I'm not totally against supporting urlopen but I think it should rather error out (with a message that one should use HDUList.fromstring(whatever.read()) than to implement it ourselves.

@MSeifert04 - i mean urllib.request.Request, which is something you can pass to urllib.request.urlopen. But actually I read Request instead of Response in your comment above, I think this explain the confusion.

To clarify:

• the current PR allows to pass a http.client.HTTPResponse to fits.open. This is returned by urlopen, and is almost file-like (it misses the seek and tell methods). As fits.open supports file-like objects, and as using HDUList.fromstring(whatever.read()) is not easy to discover, I think it makes sense to support this.

• at some point @drdavella also added the possibility to pass a urllib.request.Request to fits.open, and I thought you were speaking about this.

@MSeifert04

I'm not totally against supporting urlopen but I think it should rather error out (with a message that one should use HDUList.fromstring(whatever.read()) than to implement it ourselves.

Couldn't this argument also apply to results from open, which is already supported? The cases seem to be analogous. Personally I think it's reasonable to add support for urlopen, especially since this behavior was explicitly requested in the issue this PR attempts to resolve.

especially since this behavior was explicitly requested in the issue this PR attempts to resolve.

Ah, I thought that issue was just about the "misleading" error. Not about supporting it (although it was mentioned that it would be easy).

from #4165 (comment):

A urlopen object has never been supported (intentionally) in the first place.

But I don't feel strong about that so if you actually want to support it I won't mind. 😄

Again, I realize I'm a biased source, but it's easy to support and it's consistent with the way that regular file objects are already supported, so I think it makes sense to add support for objects from urlopen. It also seems like it's hopefully useful to someone out there (based on the SO question). It's already implemented now, and it works, so I say leave it.

But I'm new here, so I'll leave the final decision to someone else 🤐

@MSeifert04 - With the full quote it suggests that it could be supported:

A urlopen object has never been supported (intentionally) in the first place. But it could be supported easily so long as the file is opened in readonly (or denywrite) mode. It just needs to be handled explicitly. In any case the error message one gets is also misleading:

Also the issue was triggered by https://stackoverflow.com/questions/32643206/how-do-i-open-a-fits-file-from-a-url-in-astropy where someone tries to do exactly this.

So, I think everybody agree that we spent already too much time discussing this 😉. It's not a major feature but can be useful for specific cases. Let's merge and move forward (is it a special day where everybody reports FITS issues ? 😮 ).

And thanks @drdavella !