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

Fix gammapy download for Python 2 #1836

Merged
merged 1 commit into from Oct 1, 2018

Conversation

Projects
None yet
2 participants
@Bultako
Member

Bultako commented Sep 26, 2018

This PR fixes multiprocess download in Python 2.7
https://travis-ci.org/gammapy/gammapy/jobs/433184895#L1956

It also disables unicode-literals warnings in CLI apps
http://click.pocoo.org/5/python3/#unicode-literals

@Bultako Bultako self-assigned this Sep 26, 2018

@Bultako Bultako added the feature label Sep 26, 2018

@Bultako Bultako added this to the 0.8.1 milestone Sep 26, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 26, 2018

@Bultako - Thanks!

Overall I think this code would separate by splitting the functionality in two classes.

One that has our specific stuff of figuring out which files to download.

A second one that is a general ParallelDownload of files, i.e. has the code for the URL fetch, file write, parallel, progressbar.

The first class would compute the "manifest" or "request" or "plan" of files to download, and pass it to the second class to execute.

At the moment the complexity of the one class is pretty high, and I think that way to structure the code could make it a little easier to read and maintain.

Concerning folder creation, I still think a first phase where folders are created, and only then the parallel download and file write might be better. We want bullet-proof here without the possibility of race conditions. Not works 99% of the time, but then sometimes fails with weird error for some users.

You don't have to do it now, I'm interested to review and pair-code on that part of the code next week.

@cdeil cdeil modified the milestones: 0.8.1, Madrid Oct 1, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Oct 1, 2018

@Bultako - Merge? (and start new work from new PRs this week)

@Bultako Bultako merged commit 585d723 into gammapy:master Oct 1, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 3030 new issues, 1 updated code elements – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@Bultako Bultako deleted the Bultako:download-python27 branch Oct 1, 2018

@cdeil cdeil modified the milestones: Madrid, 0.9 Oct 7, 2018

@cdeil cdeil changed the title from Fix multiprocess download in 2.7 / unicode warnings to Fix gammapy download for Python 2 Nov 23, 2018

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