Skip to content

Conversation

@ericmjl
Copy link
Contributor

@ericmjl ericmjl commented Jul 11, 2024

  • Introduce the ability to pass arbitrary storage options to the underlying fsspec S3FileSystem in board_s3.
  • This enhancement allows for greater flexibility when connecting to S3-compatible services by enabling the specification of custom credentials, endpoints, and other S3FileSystem parameters.
  • Added documentation and examples to illustrate how to use the new storage_options parameter, including an example for connecting to Backblaze B2.

This change enables users to more easily integrate with a variety of S3-compatible storage solutions, improving the library's versatility and user experience.

Address issue: #235

Tag @isabelizimm!

ericmjl added 4 commits July 11, 2024 02:52
- Introduce the ability to pass arbitrary storage options to the underlying fsspec S3FileSystem in `board_s3`.
- This enhancement allows for greater flexibility when connecting to S3-compatible services by enabling the specification of custom credentials, endpoints, and other S3FileSystem parameters.
- Added documentation and examples to illustrate how to use the new `storage_options` parameter, including an example for connecting to Backblaze B2.

This change enables users to more easily integrate with a variety of S3-compatible storage solutions, improving the library's versatility and user experience.
- Replace `**kwargs` with `**storage_options` to accurately reflect the intended parameter in `board_s3` function, ensuring the correct handling of storage options.
- This commit adds an import statement for the `pins` module in the docstring example of the `board_s3` function. This change ensures that the example is self-contained and can be executed without prior context, improving the documentation's usability for new users.
- Adjusted the function definition of `board_s3` to span multiple lines, improving code readability and maintainability.
- Ensured consistency with project coding standards for function definitions.
@ericmjl
Copy link
Contributor Author

ericmjl commented Jul 11, 2024

@isabelizimm @machow I think I'm running into an issue with the docs, but I'm not quite sure how to address the issue, as it uses quartodoc (which I'm happy to learn, but may need help getting up-to-speed on).

In case it's helpful, this is a direct link to the exact line that is failing.

@isabelizimm
Copy link
Collaborator

isabelizimm commented Jul 15, 2024

Ah thank you so much for putting up this PR! Let me peek at the docs failures and run the tests on other backends to see if something jumps out 👀

update: These tests + docs failures should be fixed in #242; needed a bit of maintenance for the latest fsspec version that reordered some protocol names 👍

@isabelizimm
Copy link
Collaborator

One small nit, but everything else looks great! 😄

@ericmjl
Copy link
Contributor Author

ericmjl commented Jul 18, 2024

Woot! Looks like all of the non-skipped stuff passed 😄.

- Implemented a warning for users setting `listings_expiry_time` to a non-zero value in `board_s3` function to alert them about potential unexpected cache behaviour.
- Refactored the handling of `storage_options` to ensure `listings_expiry_time` is explicitly set, either by the user or to a default of 0, to improve clarity and maintainability of the code.

This change aims to guide users towards optimal performance settings and enhance the reliability of cache operations with S3 boards.
@ericmjl
Copy link
Contributor Author

ericmjl commented Jul 19, 2024

Okie dokes, just pushed the requested change, @isabelizimm!

Copy link
Collaborator

@isabelizimm isabelizimm left a comment

Choose a reason for hiding this comment

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

Spectacular! This is so great to expand the options for S3 pins users 🔥

@isabelizimm isabelizimm merged commit 5c929ce into rstudio:main Jul 19, 2024
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.

2 participants