remove call to system-wide gunzip. Use python version instead. by erwanp · Pull Request #1613 · astropy/astroquery

erwanp · 2020-01-09T12:57:04Z

Fixes #1538

pep8speaks · 2020-01-09T12:57:08Z

Hello @erwanp! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-16 06:06:23 UTC

erwanp · 2020-01-11T10:44:27Z

@keflavich you may want to have a look!

keflavich · 2020-01-11T15:01:28Z

Looks good at a glance. Do we have any tests for this? If not, are there any tests you could add?

Thanks!

erwanp · 2020-01-11T16:50:49Z

The gunzip utility is only used by the eso module. Could you tag anyone from there that could provide test files?

keflavich · 2020-01-11T22:19:30Z

I'm not sure we have any regular contributors to the ESO package at the moment, but that's a good thought. Maybe we can at least make some sort of round-trip test to create, zip, and unzip a file... otherwise we need to come up with some example from ESO that returns a zip. I don't know of any off the top of my head.

codecov · 2020-01-13T11:13:40Z

Codecov Report

Merging #1613 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1613      +/-   ##
==========================================
+ Coverage   62.45%   62.47%   +0.02%     
==========================================
  Files         187      187              
  Lines       15036    15025      -11     
==========================================
- Hits         9390     9387       -3     
+ Misses       5646     5638       -8

Impacted Files	Coverage Δ
astroquery/utils/system_tools.py	`54.16% <100.00%> (+8.45%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa9a34b...df3475a. Read the comment docs.

erwanp · 2020-01-13T11:13:45Z

I ran the ESO tests locally but I didn't get any zipped file there.
I added a test_system_tools.py file that does a round-trip test with a dummy file
I don't know if there is a dedicated way in pytest to remove dummy files, so I did it with the usual os/os.path functions

keflavich

Looks good. The best way to deal with files like this is to use tempfile, which handles file creation/deletion for you in a system-independent way (I think, anyway... at least, it should find a directory where you have write access)

keflavich · 2020-01-13T13:22:11Z

+from ..system_tools import gunzip
+
+def test_gunzip():
+    filename = 'test_system_tools.txt.gz'


you can use tempfile instead, e.g.:

filehandle = tempfile.NamedTemporaryFile(suffix='.txt.gz') filename = filehandle.name

keflavich · 2020-01-13T14:00:22Z

We just have some minor style fixes (see #1613 (comment) above, which was edited for your last PR) to get travis to go green, then I'll merge

keflavich · 2020-01-13T14:21:32Z

OK this is weird - @bsipocz , could you have a look? pep8speaks reports no errors, but travis is reporting:
astroquery/utils/system_tools.py:14:1: E302 expected 2 blank lines, found 1

Is this a bug in our configurations, i.e., is pep8 ignoring something that travis isn't?

keflavich · 2020-01-13T19:45:36Z

There's a travis failure that appears unrelated to this PR. I think this is good to merge, but let's hear from @bsipocz first to see if she has any ideas about the travis install failure

bsipocz

A changelog entry wouldn't hurt. I also have minor comments. I'll address them straight away, so I'll get closer to get that next 0.4 tag out.

bsipocz · 2020-01-23T03:47:27Z

+    import shutil
+    import gzip
+
+    # @EP 09/01/20: system-wide 'gzip' was removed, Python gzip used instead.


no need to date/sign comments. The git history/blame is more reliable to trace things back.

bsipocz · 2020-01-23T03:49:09Z

+"""
+
+# STDLIB
+import gzip


A bit bike shedding, but now that gzip is assumed to be optional, we need to make sure to skip the test if it's not available.

bsipocz · 2020-01-23T03:49:23Z

+import tempfile
+
+# THIRD-PARTY
+import pytest


unused import.

bsipocz · 2020-01-23T03:58:26Z

I'm now a bit confused. Is the python gzip is available on all platforms? This seems to assume it's not.

bsipocz · 2020-01-23T04:00:53Z

The travis failure was unrelated. The one remaining failure will be addressed by #1607

Also, my commit might not be neccessary, but if we revert it, then the imports also need to be moved up to the top of the file, and some more comments removed, too.

bsipocz · 2020-01-23T05:32:31Z

The windows test failure is related, maybe @pllim has some insights.

pllim · 2020-01-23T16:02:39Z

Re: Windows -- I think it is the same issue that @astrofrog encountered (and fixed) in astropy/astropy#9874

erwanp · 2020-04-06T14:12:17Z

Re: Windows -- I think it is the same issue that @astrofrog encountered (and fixed) in astropy/astropy#9874

The error is a Permission error on the Windows version of Travis so it doesn't seem related to astropy/astropy#9874 (which was pytest related). Details:

PermissionError: [Errno 13] Permission denied: 'C:\Users\travis\AppData\Local\Temp\tmp0yd2zq3f.txt.gz'

I looked up for Permissions + Windows Travis-cI for for about 40min with no luck. However, I found another ocurrence of tempfile in test_utils/test_filecontainer_save() where a temp folder is used instead of a tempfile, and that seems to be executed with no problem.

I re-wrote our test here to use a tempfolder, we'll see if that helps.

erwanp · 2020-04-08T01:42:37Z

Ended up with a KeyError: 'independent_celestial_slices' error that does not seem related to this Issue. I tried to merge the latest changes (will clean the commits at the end) but that didn't help neither. On hold until further notice.

astrofrog · 2020-04-08T09:28:56Z

The KeyError: 'independent_celestial_slices' error comes from reproject and should be fixed in reproject 0.7

bsipocz · 2020-04-08T20:44:00Z

@erwanp - could you please rebase? That would get rid of the merge commit, and retrigger CI. (I'm still not certain why the old version of reproject is picked up, rather than the new 0.7)

erwanp · 2020-04-08T22:17:12Z

Done, still get the reproject error.

…stropy#1538

…content

… it will fix Permission Errors on Travis Windows also added a comment on the 'fz' format

bsipocz · 2020-04-16T06:49:13Z

Thank you @erwanp!

saimn · 2020-09-07T19:59:31Z

While looking at reading .fits.Z with astropy (astropy/astropy#10714) I remembered about this and was curious to see how astroquery managed those files.

Before it was done by gzip which knows how to decompress .Z files.
But now, Python's gzip cannot decompress .Z files so this is broken. Testing with a file from Get error when trying to download data using astroquery.eso #1580 raises a BadGzipFile: Not a gzipped file (b'\x1f\x9d') exception.

bsipocz · 2020-09-09T19:12:21Z

thanks for checking @saimn! Would you mind opening an issue about it?

astropy-bot Bot added the utils label Jan 9, 2020

keflavich reviewed Jan 13, 2020

View reviewed changes

bsipocz added this to the v0.4 milestone Jan 23, 2020

bsipocz approved these changes Jan 23, 2020

View reviewed changes

bsipocz added the no-changelog-entry-needed label Jan 23, 2020

bsipocz force-pushed the fix_gzip_not_found branch from 8c2eb21 to 5a1d7a9 Compare January 23, 2020 04:06

bsipocz modified the milestones: v0.4, v0.4.1 Feb 3, 2020

erwanp mentioned this pull request Apr 6, 2020

'gzip was not found on your system! ' radis/radis#87

Closed

erwanp force-pushed the fix_gzip_not_found branch from 3af92b9 to 69f2dc2 Compare April 8, 2020 22:05

remove call to system-wide gunzip. Use python version instead. Fixes a…

5cdc707

…stropy#1538

erwanp and others added 7 commits April 15, 2020 23:05

fix indentation for PEP 8

2196cb2

round-trip test that we can zip/unzip a dummy file and keep the same …

9fe0e79

…content

use tempfile for file generation

c416152

autopep8

b5163f3

still more pep8!

14d21fb

Minor cleanup

8e800fe

rewrote test_gunzip to use a tempfolder instead of a tempfile, hoping…

df3475a

… it will fix Permission Errors on Travis Windows also added a comment on the 'fz' format

bsipocz force-pushed the fix_gzip_not_found branch from 69f2dc2 to df3475a Compare April 16, 2020 06:06

bsipocz merged commit 815d2e5 into astropy:master Apr 16, 2020

saimn mentioned this pull request Sep 10, 2020

ESO: decompressing .fits.Z files is broken #1818

Closed

bsipocz mentioned this pull request Feb 24, 2022

Refactor astroquery/utils/system_tools.py and its tests #2306

Merged

Conversation

erwanp commented Jan 9, 2020

Uh oh!

pep8speaks commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-04-16 06:06:23 UTC

Uh oh!

erwanp commented Jan 11, 2020

Uh oh!

keflavich commented Jan 11, 2020

Uh oh!

erwanp commented Jan 11, 2020

Uh oh!

keflavich commented Jan 11, 2020

Uh oh!

codecov Bot commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

erwanp commented Jan 13, 2020

Uh oh!

keflavich left a comment

Choose a reason for hiding this comment

Uh oh!

keflavich Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

erwanp Jan 13, 2020

Choose a reason for hiding this comment

Uh oh!

keflavich commented Jan 13, 2020

Uh oh!

keflavich commented Jan 13, 2020

Uh oh!

keflavich commented Jan 13, 2020

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

bsipocz Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

bsipocz Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Jan 23, 2020

Uh oh!

bsipocz commented Jan 23, 2020

Uh oh!

bsipocz commented Jan 23, 2020

Uh oh!

pllim commented Jan 23, 2020

Uh oh!

erwanp commented Apr 6, 2020

Uh oh!

erwanp commented Apr 8, 2020

Uh oh!

astrofrog commented Apr 8, 2020

Uh oh!

bsipocz commented Apr 8, 2020

Uh oh!

erwanp commented Apr 8, 2020

Uh oh!

bsipocz commented Apr 16, 2020

Uh oh!

saimn commented Sep 7, 2020

Uh oh!

bsipocz commented Sep 9, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pep8speaks commented Jan 9, 2020 •

edited

Loading

codecov Bot commented Jan 13, 2020 •

edited

Loading