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

Use fsspec for fetching external testing files #178

Merged
merged 8 commits into from
Jul 26, 2021

Conversation

gadomski
Copy link
Member

@gadomski gadomski commented Jul 23, 2021

Related Issue(s): n/a

Description: This enables external files stored on all of the systems supported by fsspec.

Also includes:

  • Two extra_requires options. stactools[s3] installs s3fs, which is required to use fsspec on s3 hrefs. stactools[all] includes s3 and any future extra_requires.
  • CI tests for stactools[all], to make sure we are testing both with and without extra_requires.
  • Version bump for fssepc.
  • Unit tests for stactools.testing, which leads to saying "test" a lot :-).
  • README instructions for install w/ optional dependencies.

PR checklist:

  • Code is formatted (run scripts/format).
  • Code lints properly (run scripts/lint).
  • Tests pass (run scripts/test).
  • Documentation has been updated to reflect changes, if applicable.
  • Changes are added to the CHANGELOG.

This enables external files stored on all of the systems supported by
fsspec.

Also includes:
- Two `extra_requires` options. `stactools[s3]` installs `s3fs`, which
  is required to use fsspec on s3 hrefs. `stactools[all]` includes `s3`
  and any future extra_requires.
- CI tests for `stactools[all]`, to make sure we are testing both with
  and without `extra_requires`.
- Version bump for `fssepc`.
- Unit tests for `stactools.testing`, which leads to saying "test" a lot
  :-).
- README instructions for install w/ optional dependencies
@gadomski gadomski requested a review from cuttlefish July 23, 2021 18:34
@gadomski gadomski added the enhancement New feature or request label Jul 23, 2021
@gadomski gadomski marked this pull request as draft July 23, 2021 19:09
@gadomski
Copy link
Member Author

Converting to draft because I'm going to add some kwargs to the external data structure, as I need to use an endpoint url as documented here: https://s3fs.readthedocs.io/en/latest/#self-hosted-s3.

@gadomski gadomski marked this pull request as ready for review July 23, 2021 19:23
@gadomski gadomski force-pushed the feature/external-data-with-fsspec branch from f49d32e to da35a91 Compare July 23, 2021 19:27
There's a package named `s3` so I couldn't use a dependency chain :-(.
The GOES data needed anon=True, and there's no way to default to that
w/o an s3 config object, which we're already testing, so get it out of
here.
@cuttlefish
Copy link
Collaborator

cuttlefish commented Jul 23, 2021

Nice! This will be really useful.
I won't hold up this PR, but I'll add it to the Docker images.
pip install stactools[all] ended up not working, right?
I pushed fixes for both.

@cuttlefish
Copy link
Collaborator

@gadomski Sorry for messing with your branch, but I updated the Dockerfile so that I could test this. I also got the all extras_require working, although I'm not sure how robust it is.

Copy link
Collaborator

@cuttlefish cuttlefish left a comment

Choose a reason for hiding this comment

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

Looks good!

setup.cfg Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@gadomski gadomski added this to the v0.2.1 milestone Jul 26, 2021
@cuttlefish cuttlefish merged commit 228d260 into main Jul 26, 2021
cuttlefish pushed a commit that referenced this pull request Jul 26, 2021
@cuttlefish cuttlefish deleted the feature/external-data-with-fsspec branch July 26, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants