-
Notifications
You must be signed in to change notification settings - Fork 49
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
Make startup scripts & downloading of files more robust #507
Make startup scripts & downloading of files more robust #507
Conversation
Fix issue panoptes#506 by making the downloading code ignore failures to download files.
Codecov Report
@@ Coverage Diff @@
## develop #507 +/- ##
===========================================
- Coverage 70.48% 70.32% -0.17%
===========================================
Files 62 62
Lines 5418 5418
Branches 751 752 +1
===========================================
- Hits 3819 3810 -9
- Misses 1392 1398 +6
- Partials 207 210 +3
Continue to review full report at Codecov.
|
plot_weather.py writes into /var/panoptes/plot_weather/, but there is nothing that creates that directory, so this commit adds that to plot_weather.py. Running python directly from bash command line in a crontab rule is failing, so this commit adds two shell scripts for doing so: plot_weather.sh and update_indices.sh. Updates README.md accordingly. Updates the 'Current user:' lines in some scripts to use the whoami command rather than $USER. It turns out that Posix specifies $LOGNAME (i.e. login name) as the envvar to be set to the name of the logged in user by the login command, not $USER.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some comments that should be looked at but will go ahead and approve now. Thanks!
pocs/utils/data.py
Outdated
_suppress_IERS_A_warning.original(message, *args, **kwargs) | ||
|
||
|
||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused as to this setup. Why not just use the regular method of suppressing warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd never seen that method for suppressing warnings, thanks. I'll update accordingly.
pocs/utils/data.py
Outdated
|
||
from astroplan import download_IERS_A | ||
from astropy.utils import data | ||
def _default_data_folder(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a function or could it just be a global variable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to a global var.
pocs/utils/data.py
Outdated
""" | ||
Args: | ||
data_folder: Path to directory into which to copy the astrometry.net indices. | ||
wide_field: If true, downloads wide field astrometry.net indices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
-> True
. Also should mark the defaults. Same with narrow_field
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
pocs/utils/data.py
Outdated
keep_going: If False, exceptions are not suppressed. If True, returns False if there | ||
are any download failures, else returns True. | ||
""" | ||
self.data_folder = data_folder or _default_data_folder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably handle folder creation.
pocs/utils/data.py
Outdated
if narrow_field: | ||
for i in range(4210, 4219): | ||
download_one_file('4200/index-{}.fits'.format(i)) | ||
def download_all_files(data_folder=None, wide_field=True, narrow_field=False, keep_going=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem necessary and seems like it could just be handled in the __main__
. Any specific reason for the wrapper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you want to keep it I would suggest a different name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the function. It was intended as a helper function for other parts of POCS that would import this file, but isn't happening anymore. In fact, we could probably move this to POCS/scripts/
pocs/utils/data.py
Outdated
action='store_true', | ||
help='Ignore download failures and keep going to the other downloads (default)') | ||
group.add_argument( | ||
'--nokeep_going', action='store_true', help='Fail immediately if any download fails') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat related to lack of convention mentioned above, I would separate the "no", i.e. --no-keep-going
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
args = parser.parse_args() | ||
|
||
if args.folder and not os.path.exists(args.folder): | ||
print("{} does not exist.".format(args.folder)) | ||
print("Warning, data folder {} does not exist.".format(args.folder)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warn or just auto-create? We create directories in a number of different places so might just make sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to auto-create.
@@ -0,0 +1,51 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of boilerplate in these bash scripts. Any way we can consolidate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is a lot of boilerplate... and some of it is even valuable! ;-)
I don't have a great solution yet, so will defer.
scripts/update_indices.sh
Outdated
@@ -0,0 +1,52 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about boilerplate above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
scripts/update_indices.sh
Outdated
@@ -0,0 +1,52 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the filename here isn't clear. Maybe something explicit like "download_support_files.sh"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks for making the changes!
return dl.download_all_files() | ||
|
||
|
||
if __name__ == '__main__': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to say that the argparse statements should be in this __name__
and that those arguments should then be passed to a main
but I can't find anything to support why that would be a preferred way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a need for that... except that I didn't pass in sys.argv[1:]
so that it could be passed to argparse.parse()
. However, the examples I found didn't bother with that. And it wouldn't make sense to call the method main
if the parsing had already occurred Anyway, we can fix that later if it makes sense.
|
||
set -x | ||
|
||
python "${POCS}/pocs/utils/data.py" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole wrapper script seems to be to guarantee that the environment is correct for downloading but it doesn't check the python
version. Should probably source activate panoptes-env
if we are still recommending that route otherwise would need to make sure python3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source activate panoptes-env
should be in $HOME/.profile, and that is what the instructions call for. After all, this is a script designed to be run from cron on a PANOPTES scope, not necessarily for a dev box.
Nonetheless, I suspect you're right that I should make such a change, along with other changes to reduce the boilerplate. Since this PR has been going for a while, I'd like to handle those changes in another PR. I filed #519 to track this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks. This PR is still approved so I'll leave to you to merge when ready.
|
||
set -x | ||
|
||
"${POCS}/scripts/plot_weather.py" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above although we should make them consistent. This one appears to be executable where the data.py
above is not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the POCS/scripts are executable, but I don't think we have many in POCS/pocs that are executable.
Fix issue #506 by making the downloading code ignore failures to
download files.