Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@gadomski
Copy link
Member

@gadomski gadomski commented Mar 30, 2022

Related Issue(s): None

Description: This is a DRAFT pull request to experiment with ideas for adding support for a class-based item builder.

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.

@gadomski gadomski added this to the v0.4.0 milestone Mar 30, 2022
@gadomski gadomski requested a review from matthewhanson March 30, 2022 21:19
@gadomski gadomski force-pushed the refactor branch 3 times, most recently from 15ecb34 to b9f5384 Compare March 30, 2022 21:33
@gadomski gadomski changed the title Large-scale refactor Datasets Mar 30, 2022
@gadomski gadomski force-pushed the refactor branch 2 times, most recently from 80d492b to 03459ce Compare April 4, 2022 20:43
@gadomski gadomski changed the title Datasets Item builder Apr 4, 2022
@gadomski gadomski force-pushed the refactor branch 4 times, most recently from 27ec0e4 to 95d5237 Compare April 7, 2022 21:26
@gadomski gadomski requested a review from pjhartzell April 7, 2022 21:27
@gadomski gadomski force-pushed the refactor branch 9 times, most recently from b50151e to b5d0d4b Compare April 7, 2022 22:42
Copy link
Collaborator

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

I mentioned this on a call earlier, but this is a pattern built on fsspec that I've found helpful.

https://github.com/TomAugspurger/era5/blob/main/src/stactools/era5/stac.py#L150-L155

The create_item includes the protocol and storage_options keywords.

def create_item(
    path: str,
    kind: str,
    protocol: str,
    storage_options: dict[str, Any] | None = None,
) -> pystac.Item:

And then the usage internally is

    storage_options = storage_options or {}
    fs = fsspec.filesystem(protocol, **storage_options)

And all paths are relative to the dataset root. All path-like operations within create_item would use fsspec. In theory, this lets you use the same code for STAC items that might live on different filesystems (S3, Azure Blob Storage, local, ...). Hope that's helpful :)

from pystac import Asset

builder = Builder()
builder.add_asset("an-asset", Asset(href="a-file.dat"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style question: what's the motivation for builder = Builder(); builder.add_asset pattern rather than defining what Builder.create_item will do in the class definitation?

Copy link
Member Author

@gadomski gadomski Apr 19, 2022

Choose a reason for hiding this comment

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

Partially, it's my procedural background, but I do find that leaving the asset addition until later is useful for conditionals, see here: https://github.com/stactools-packages/modis/blob/524bfd26bb6d571336e50a226e853b97287529e3/src/stactools/modis/commands.py#L74

@gadomski
Copy link
Member Author

gadomski commented Jun 6, 2022

Closing as will-reimplement -- this needs to be simpler.

@gadomski gadomski closed this Jun 6, 2022
@gadomski gadomski deleted the refactor branch June 9, 2022 14:33
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.