openclimatefix / Satip

Satip contains the code necessary for retrieving, transforming and storing EUMETSAT data
https://satip.readthedocs.io/
MIT License
41 stars 28 forks source link

a new parameter 'backend' to specify the fsspec backend #233

Closed aryanbhosale closed 6 months ago

aryanbhosale commented 6 months ago

This modification introduces a new parameter backend to specify the fsspec backend. Note that you need to replace "" in fsspec.open function with the appropriate token for the selected backend.

Pull Request

Description

Please delete the italicised instruction text! Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

Checklist:

aryanbhosale commented 6 months ago

Hi @jacobbieker , could you please review my commits? Thank you :)

aryanbhosale commented 6 months ago

Thanks! This looks great overall! Just one minor change if you can

Sure! Will make the changes

aryanbhosale commented 6 months ago

@jacobbieker I also noticed that there are 2 places in the utils.py file that use s3 bucket urls, wouldn't that also change dynamically as the user adds their own cloud platforms? What change do you recommend done there? line 1062: hrv_files = ["zip:///::s3://" + str(f) for f in hrv_files] line 1099: nonhrv_files = ["zip:///::s3://" + str(f) for f in nonhrv_files]

jacobbieker commented 6 months ago

@jacobbieker I also noticed that there are 2 places in the utils.py file that use s3 bucket urls, wouldn't that also change dynamically as the user adds their own cloud platforms? What change do you recommend done there? line 1062: hrv_files = ["zip:///::s3://" + str(f) for f in hrv_files] line 1099: nonhrv_files = ["zip:///::s3://" + str(f) for f in nonhrv_files]

For those, I would suggest splitting out the file prepending into its own little function add_backend_to_filenames(files, backend) that adds the proper backend string for fsspec chaining in that. So for S3, does what is currently shown, for GCS adds zip:///::gs://" instead, for Azure, adds zip:///::az:// and for local, doesn't add anything

aryanbhosale commented 6 months ago

@jacobbieker I also noticed that there are 2 places in the utils.py file that use s3 bucket urls, wouldn't that also change dynamically as the user adds their own cloud platforms? What change do you recommend done there? line 1062: hrv_files = ["zip:///::s3://" + str(f) for f in hrv_files] line 1099: nonhrv_files = ["zip:///::s3://" + str(f) for f in nonhrv_files]

For those, I would suggest splitting out the file prepending into its own little function add_backend_to_filenames(files, backend) that adds the proper backend string for fsspec chaining in that. So for S3, does what is currently shown, for GCS adds zip:///::gs://" instead, for Azure, adds zip:///::az:// and for local, doesn't add anything

Oh that sounds great. On it!

aryanbhosale commented 6 months ago

@jacobbieker I also noticed that there are 2 places in the utils.py file that use s3 bucket urls, wouldn't that also change dynamically as the user adds their own cloud platforms? What change do you recommend done there? line 1062: hrv_files = ["zip:///::s3://" + str(f) for f in hrv_files] line 1099: nonhrv_files = ["zip:///::s3://" + str(f) for f in nonhrv_files]

For those, I would suggest splitting out the file prepending into its own little function add_backend_to_filenames(files, backend) that adds the proper backend string for fsspec chaining in that. So for S3, does what is currently shown, for GCS adds zip:///::gs://" instead, for Azure, adds zip:///::az:// and for local, doesn't add anything

I've added the required changes in my latest commit, could you please review them ?

aryanbhosale commented 6 months ago

This looks good! Thanks for all this!

Thank you @jacobbieker !