Skip to content
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

add file download functinality to ERDDAP #330

Merged
merged 7 commits into from
Jan 20, 2024

Conversation

callumrollo
Copy link
Contributor

@callumrollo callumrollo commented Jan 12, 2024

This adds a basic method to download files from erddap with a user specified file extension.

Envisaged usage:

from erddapy import ERDDAP
e = ERDDAP("https://erddap.observations.voiceoftheocean.org/erddap")
e.dataset_id = "nrt_SEA068_M27"
e.protocol="tabledap"
e.variables=["depth", "time"]
e.download_file("mat")

This enables the usage of existing subsetting and constraints.

Limitations:

  • currently saves files just as <dataset_id>.<extension>
  • will not re-download file if/when constraints are changed
  • no way to handle invalid file type requests
  • no tests yet!

@ocefpaf
Copy link
Member

ocefpaf commented Jan 15, 2024

will not re-download file if/when constraints are changed

We could compute a hash based on the constraints and add it to the file name. That could be useful for folks who will download multiple files requests from the same dataset_id programatically.

no way to handle invalid file type requests

What do you mean? Are you counting with ERDDAP's error message? Maybe we can restrict this to valid known file types.

erddapy/erddapy.py Outdated Show resolved Hide resolved
@callumrollo
Copy link
Contributor Author

Good idea with the hash, I've added that functionality.

Currently, if you request a filetype that ERDDAP does not have, it will send the request and return an HTTP 400 error. Is this a desired outcome? Or would it be better to catch with a pre-defined list of allowed filetypes? Somthing like:

if file_type not in erdddap_filetypes_list:
            raise ValueError(
                f"Requested filetype {file_type} not available on ERDDAP",
            )

@callumrollo
Copy link
Contributor Author

Looks like the IOOS ERDDAP is down, which is crashing the tests. I'll add more tests for this functionality

@callumrollo callumrollo marked this pull request as ready for review January 16, 2024 09:52
@ocefpaf
Copy link
Member

ocefpaf commented Jan 16, 2024

Looks like the IOOS ERDDAP is down, which is crashing the tests. I'll add more tests for this functionality

Pinging @MathewBiddle here. Maybe it is a scheduled maintenance.

@ocefpaf
Copy link
Member

ocefpaf commented Jan 17, 2024

@callumrollo everything is back online now. Thanks @MathewBiddle for getting the server back up.

@ocefpaf
Copy link
Member

ocefpaf commented Jan 17, 2024

Good idea with the hash, I've added that functionality.

That the good thing about working with others. I would never thought of that but, because you raised the issue, I had to think of a solution.

Currently, if you request a filetype that ERDDAP does not have, it will send the request and return an HTTP 400 error. Is this a desired outcome? Or would it be better to catch with a pre-defined list of allowed filetypes?

I'm inclined to the latter b/c the former, while more elegant, is not consistent. Not all ERDDAP servers return the same error code. @abkfenris has some code to catch that if you want to try but I'm inclined to just create a list of allowed file types for download.

@abkfenris
Copy link
Contributor

Not all ERDDAP servers return the same error code. @abkfenris has some code to catch that if you want to try but I'm inclined to just create a list of allowed file types for download.

I've recently refactored the error handling out into it's own file a bit better if you want an idea of what that might look like: https://github.com/gulfofmaine/buoy_barn/blob/main/app/deployments/tasks/error_handling.py

erddapy/erddapy.py Outdated Show resolved Hide resolved
@ocefpaf
Copy link
Member

ocefpaf commented Jan 18, 2024

Looks great! Thanks @callumrollo!! Do you want to add anything else or should we merge it?

@callumrollo
Copy link
Contributor Author

Looks good to me! Merge away

@ocefpaf ocefpaf merged commit 829f586 into ioos:main Jan 20, 2024
16 checks passed
@ocefpaf
Copy link
Member

ocefpaf commented Jan 20, 2024

Thanks @callumrollo! This turned out way better than what I wanted to implement.

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.

3 participants