search

MWATelescope / mwa_pb

MWA Primary Beam model code
MIT License
2 stars 8 forks source link

Instead of using git-lfs, config.py downloads the h5 file #13

Closed NickSwainston closed 3 years ago

mwa-site commented 4 years ago

Hi Nick, that fix is only going to work if the person running the code that imports mwa_pb has write permission in the directory where mwa_pb is installed. That'll be true if it's installed in their home directory in a local python environment, but won't be in a lot of other cases, like on Galaxy, or the Django servers running the web services.

Why not put the code to download the file in setup.py, so it happens before the package is installed?

Andrew

On 2020-07-21 1:49 PM, Nick Swainston wrote:

@NickSwainston https://github.com/NickSwainston requested your review on: #13 https://github.com/MWATelescope/mwa_pb/pull/13 Instead of using git-lfs, config.py downloads the h5 file.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/MWATelescope/mwa_pb/pull/13#event-3569280762, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJV4YGTM3EYK4TT554I2E2DR4UT5VANCNFSM4PDFVEGA.

NickSwainston commented 4 years ago

After doing the usual google search of how to solve the problem, I found this stake overflow page: https://stackoverflow.com/questions/37513279/using-setuptools-how-can-i-download-external-data-upon-installation It warns "Python package installs should be completely reproducible, and any potential external-download issues should be pushed to runtime" so I wrote the script in the above way.

The write permission issue can be solved by running python -c "import mwa_pb.config" the first time it is installed (by someone with the correct permissions). If you would still prefer the h5 file to be downloading during installation I can move it to setup.py if you wish.

Nick

NickSwainston commented 4 years ago

After some discussion, we decided the best implementation would be to download the h5 file before the setup function so it will be wheeled into the dist files and uploaded to pypi to assure the installation works with all methods.

I shall merge the changes when the review is approved.

andreww5au commented 3 years ago

Sorry Nick, I didn't notice this. I've just merged the change.