Fix audmodel.url() for Minio backends#54
Conversation
Reviewer's GuideThis pull request refactors audmodel.url() to construct absolute URLs for both filesystem and Minio/S3 backends by introducing a shared helper and expanding tests to validate URL behavior, including a new Minio-specific test. Sequence diagram for updated audmodel.url() URL constructionsequenceDiagram
actor Client
participant audmodel_api as audmodel_api
participant backend_interface as backend_interface
participant backend as backend
Client->>audmodel_api: url(uid, type, version, backend_interface)
audmodel_api->>backend_interface: _path_with_version(path, version)
backend_interface-->>audmodel_api: backend_path
audmodel_api->>audmodel_api: _url(backend_interface, backend_path)
audmodel_api->>backend_interface: sep.join(parts)
audmodel_api->>backend_interface: backend
alt [FileSystem backend]
audmodel_api->>backend: _root
audmodel_api->>backend_interface: sep.join([backend._root, backend_path])
else [Minio backend]
audmodel_api->>backend: host
audmodel_api->>backend: repository
audmodel_api->>backend: path(backend_path)
audmodel_api->>backend_interface: sep.join([scheme_host, backend.repository, backend.path(backend_path)])
end
audmodel_api-->>Client: absolute_url
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
_url()helper relies heavily on private attributes and methods of the backend (e.g.,_root,_client._base_url,path), which may be brittle; consider encapsulating this logic in audbackend or exposing public helpers instead of reaching into internals. _url()silently returns the unmodified path for backend types other than FileSystem and Minio; if additional backends are expected, consider making the behavior explicit (e.g., raising, or adding a clear default handling) to avoid ambiguous results.- The Minio URL construction assumes a specific host format and scheme derivation; it might be safer to reuse existing URL-building utilities or centralize this logic so changes to Minio configuration or client behavior don’t require touching audmodel.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_url()` helper relies heavily on private attributes and methods of the backend (e.g., `_root`, `_client._base_url`, `path`), which may be brittle; consider encapsulating this logic in audbackend or exposing public helpers instead of reaching into internals.
- `_url()` silently returns the unmodified path for backend types other than FileSystem and Minio; if additional backends are expected, consider making the behavior explicit (e.g., raising, or adding a clear default handling) to avoid ambiguous results.
- The Minio URL construction assumes a specific host format and scheme derivation; it might be safer to reuse existing URL-building utilities or centralize this logic so changes to Minio configuration or client behavior don’t require touching audmodel.
## Individual Comments
### Comment 1
<location path="audmodel/core/api.py" line_range="1071-1080" />
<code_context>
+def _url(
</code_context>
<issue_to_address>
**issue (bug_risk):** Behavior for unsupported backend types may not match the function’s URL-centric contract.
The docstring says `_url` converts a backend path into a URL, but for backends other than `FileSystem` and `Minio` it returns the raw `path`. This can mask misconfigurations or new backend types that aren’t wired up correctly. Consider failing explicitly (e.g., raising for unsupported backends), returning `None`, or updating the docs to state that non-URL-capable backends return the backend-relative path unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def _url( | ||
| backend_interface: audbackend.interface.Base, | ||
| path: str, | ||
| ) -> str: | ||
| r"""Convert a backend path into a URL. | ||
|
|
||
| Depending on the underlying backend | ||
| of the given backend interface, | ||
| the path on the backend | ||
| is turned into a URL |
There was a problem hiding this comment.
issue (bug_risk): Behavior for unsupported backend types may not match the function’s URL-centric contract.
The docstring says _url converts a backend path into a URL, but for backends other than FileSystem and Minio it returns the raw path. This can mask misconfigurations or new backend types that aren’t wired up correctly. Consider failing explicitly (e.g., raising for unsupported backends), returning None, or updating the docs to state that non-URL-capable backends return the backend-relative path unchanged.
We have
audmodel.url()which returns a link to the actual physical location of the model. It worked under filesystem and artifactory, but never under Minio for which it just returned the relative path on the server. But you can also create absolute path for Minio/S3 as well.This pull request introduces changes the behavior of
audmodel.url()to return absolute paths. It uses internal hidden methods fromaudbackend, but we need to do the same for filesystem as long as we do not want to extendaudbackend.This is a breaking change, but I would call it a fix.