Skip to content

Move download script to root directory#9

Merged
bouweandela merged 9 commits intomasterfrom
move_download_script
Dec 1, 2020
Merged

Move download script to root directory#9
bouweandela merged 9 commits intomasterfrom
move_download_script

Conversation

@stefsmeets
Copy link
Contributor

This PR moves the download script to the root directory and gives it a more logical name.

@stefsmeets stefsmeets marked this pull request as ready for review November 18, 2020 11:39
@nielsdrost
Copy link
Member

nielsdrost commented Nov 18, 2020

I tried to run this script but ran into some trouble.

Then I gave up for today ;-)

@stefsmeets
Copy link
Contributor Author

I tried to run this script but ran into some trouble.

* This error [ESGF/esgf-pyclient#57](https://github.com/ESGF/esgf-pyclient/issues/57), solved by adding a few lines at the top to set the SSL version (see issue)

* The config file and dataset.yml locations do not seem to be correctly specified

* I get a "pyesgf.search.exceptions.EsgfSearchException: Error loading available shards" error.

Then I gave up for today ;-)

Thanks @nielsdrost , good points and it seems like I should have tested this myself first 😅

@stefsmeets
Copy link
Contributor Author

The esgf issues I cannot help with, but I can make sure that datasets.yml is correctly referenced. Should work as intended now (or at least it does so on my pc).

@stefsmeets stefsmeets linked an issue Nov 19, 2020 that may be closed by this pull request
11 tasks
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The download script and its configuration file (datsets.yml) belong together, so if you're moving one to the top level, I would also move the other to the top level.

@stefsmeets
Copy link
Contributor Author

Hi @bouweandela , can you have another look? I think this is ready to merge!

Copy link
Member

@bouweandela bouweandela left a 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, I tried to run the script but it failed on loading the configuration file. I also noticed that the documentation on how run the script in CONTRIBUTING.md has not been updated yet.

@stefsmeets
Copy link
Contributor Author

Thanks for making the changes, I tried to run the script but it failed on loading the configuration file. I also noticed that the documentation on how run the script in CONTRIBUTING.md has not been updated yet.

Yeah, my bad for not testing if it worked. Should be good now 👍

@bouweandela bouweandela merged commit eaa9d59 into master Dec 1, 2020
@bouweandela bouweandela deleted the move_download_script branch December 1, 2020 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support tests for multimodel statistics preprocessor function

3 participants