Skip to content

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.

@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

@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
@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