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

1025/caching cloud #1026

Merged
merged 20 commits into from
Jul 16, 2024
Merged

1025/caching cloud #1026

merged 20 commits into from
Jul 16, 2024

Conversation

Jaapel
Copy link
Contributor

@Jaapel Jaapel commented Jul 9, 2024

Issue addressed

Fixes #1025

Explanation

Wanted to fix the AWS data catalog.
Noticed that the dataset tested used a gdal virtual filesystem. This messes with the caching settings. Of course this path is different in windows and linux, so the code had to be revised.
Then that the .vrt caching did not cache at all: the intersects in-line functions was not correct and skipped all caching.
The vrt also had to be altered to account for the cached files.

General Checklist

  • Updated tests or added new tests
  • Branch is up to date with v1
  • Tests & pre-commit hooks pass
  • Updated documentation
  • Updated changelog.rst

Additional Notes (optional)

I did change the data catalog for GCP CMIP6 data. But as we are still in alpha, i do not release a new version.

@Jaapel Jaapel self-assigned this Jul 10, 2024
@Jaapel Jaapel added the V1 label Jul 10, 2024
Copy link
Contributor

@savente93 savente93 left a comment

Choose a reason for hiding this comment

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

Looking good Jaap. I don't know enough about how vrt files work to comment very semantically but the code looks good to me. I have a few small questions, once those are resolved I'll be happy to approve

hydromt/_utils/caching.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/data_catalog/test_data_catalog.py Outdated Show resolved Hide resolved
@Jaapel Jaapel requested a review from savente93 July 15, 2024 15:14
if vrt_ref_el is None:
raise ValueError(f"Could not find Source File in vrt: {vrt_uri}.")
vrt_ref: str = vrt_ref_el.text.replace("\\", os.sep) # dewindowsify
vrt_relative: int = int(vrt_ref_el.get("relativeToVRT")) # 0 or 1
Copy link
Contributor

Choose a reason for hiding this comment

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

why int and not bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The source format gives either 0 or 1, I like to represent it that way.

Copy link
Contributor

@savente93 savente93 left a comment

Choose a reason for hiding this comment

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

LGTM

@Jaapel Jaapel merged commit e965e1d into v1 Jul 16, 2024
8 checks passed
@Jaapel Jaapel deleted the 1025/caching_cloud branch July 16, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants