Skip to content

Conversation

@j279li
Copy link
Contributor

@j279li j279li commented May 20, 2025

Refactors the Polaris Hub client and storage modules to simplify authentication handling and allow for public access of artifacts. Key changes include removing authentication for reads, and updating dataset and benchmark retrieval methods to get data path from the API response.

Authentication and Session Management Updates:

  • Removedauthentication checks for retrieval methods in PolarisHubClient and added explicit token validation where necessary, such as in upload_results and upload_model.
  • Introduced a mode parameter in StorageSession to control authentication requirements, making it optional for read operations but mandatory for write operations.

Dataset and Benchmark Handling Improvements:

  • Simplified dataset and benchmark retrieval methods by replacing _base_request_to_hub with request and using metadata fields (zarr_path, table_path) for loading resources.

Storage Refactor:

  • Replaced custom S3 storage logic for public reads in StorageSession with fsspec for handling get file and store operations.

Codebase Simplification:

  • Consolidated logic for handling Zarr archives and storage paths across dataset-related methods.

@j279li j279li self-assigned this May 20, 2025
@j279li j279li requested a review from cwognum as a code owner May 20, 2025 19:31
@j279li
Copy link
Contributor Author

j279li commented May 20, 2025

Tests right now are failing because the Hub hasn't been updated yet (as the client isn't sending authentication tokens anymore).

Copy link
Contributor

@Andrewq11 Andrewq11 left a comment

Choose a reason for hiding this comment

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

Thanks, @j279li! A lot of good work here. I've left some comments related to suggestions and clarifications that we should cover.

Also, I've requested the wisdom of @jstlaurent on some storage action organization.

Copy link
Contributor

@jstlaurent jstlaurent 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! A few questions and suggestions. 😄

Copy link
Contributor

@Andrewq11 Andrewq11 left a comment

Choose a reason for hiding this comment

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

Nice work! Left a few small comments. Looks good on my side after those.

@j279li j279li merged commit f844b4c into main May 23, 2025
19 checks passed
@cwognum cwognum deleted the refactor/public-bucket-access branch May 23, 2025 21:30
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.

4 participants