Repository navigation
Make bz2 import optional for io.fits - #9652
Conversation
|
I think this can wait for v4.0.1 (unless there is a chance to get it in v4.0, @bsipocz ?), and would classify this as bug fix. |
|
@saimn - 4.0.1 is fine and I actually prefer (keep the critical stuff in 4.0 so I see what are the blockers). Otherwise anything in 4.0.1 gets merged before 4.0 is out I'll remilestone them and ship in that release. |
|
However, for this one, I'm not sure it actually needs a changelog entry? |
|
Not having a changelog entry is also an option, it's a rare case and not really a bug in astropy. |
|
Just thought about adding a note to the |
| new_file = gzip.GzipFile(name, mode='ab+') | ||
| elif self._file.compression == 'bzip2': | ||
| if not HAS_BZ2: | ||
| raise ValueError( |
|
@dhomeier Could you address the open points? Or is there any point which needs further discussion? |
|
I think there is agreement on all the open points. |
| compressed with gzip, bzip2 or pkzip. Note that in this context we are talking | ||
| about a FITS file that has been compressed with one of these utilities (e.g., a | ||
| .fits.gz file). | ||
| .fits.gz file). Opening bzip2-compressed files requires the :mod:`bz2` module |
There was a problem hiding this comment.
This makes it sound like it's an optional dependency. I think it's reasonable to expect most users to have the module so this doesn't need to be explicitly mentioned, especially not on the main index.rst.
There was a problem hiding this comment.
Hmm, the whole point of the PR was to make it an optional dependency so users with the (rare) setup of a Python built without bz2 support would be able to use Astropy with the remaining functionality at all. It is sitting there quite prominently for such a fringe case, but I did not find any other documentation on the automatic (de)compression functionality.
There was a problem hiding this comment.
is sitting there quite prominently for such a fringe case, but I did not find any other documentation on the automatic (de)compression functionality.
Yeah, my suggestion would be to just not add it to the documentation at all.
There was a problem hiding this comment.
That would probably be the alternative, as most of the fits documentation sits in index.rst. The exception message is hopefully informative enough whenever this issue might come up in the future.
There was a problem hiding this comment.
Yes I think we don't need to mention it here, since a normal Python should come with it.
|
I think this looks good now. I'm not sure if we should squash the commits to remove the back-and-forth. What do you think @saimn? |
|
Yes, looks good, thanks @dhomeier. Squashing could be useful to clean the history, if you have time, but we can also do without. |
|
Thanks @dhomeier! |
|
Thanks @MSeifert04 for the review and everyone for their comments! |
@dhomeier , please go ahead and open a follow-up issue if you haven't already. Thanks! |
Make bz2 import optional for `io.fits`
Description
Addressing #5967: removed the unconditional
import bz2fromio.fitsso the module itself can be imported in Python installations without thebz2module. For these systems only the autodetection of bzip2'ed FITS files based on file signature and of course their transparent decompression will be disabled.Notes
I do not have a Python interpreter built without bzip2 support; thus I only made some checks for this case by forcing the import to fail (misnaming the module).
It should be possible to mark the bzip2-tests
xfailor check for the appropriate exceptions, if the_make_bzip2_filefunction for creating the test files was modified to fall back to anos.system("bzip2 -k test0.fits")call or the like. But that would probably fail on Windows, and on other systems we should perhaps not make assumptions on the availability of the command linebzip2either, so it did not seem worth the effort to me.Should this go under "API Changes" or "Bug Fixes"?