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

Better authentication handling in backends #215

Merged
merged 20 commits into from
Apr 19, 2024
Merged

Conversation

hagenw
Copy link
Member

@hagenw hagenw commented Apr 10, 2024

Closes #161, #162


* Add class method audbackend.backend.Artifactory.get_authentication()

This tackles #161 by providing a public class method to read authentication tokens from a config file or environment variables.

image


* Add authentication argument and attribute to audbackend.backend.Base

This tackles #162 by adding an authentication argument to __init__(), create(), and delete() of audbackend.backend.Base.

In audbackend.backend.FileSystem I then do not add authentication as an argument to __init__(). Don't know if it would be better to always have it, but from a user perspective I found it easier this way.


* Speedup audbackend.backend.Artifactory

audbackend.backend.Artifactory now only applies the authentication when calling open(). Before it did the authentication every time a path was requested on the backend, e.g. every time backend.exists(path) was called. Now, we store the path to the repo in self._repo, when calling open(), and can create all other paths based on top of that.

When asking if 4 files exists on the backend this reduced the execution time for me from 0.872 s to 0.178 s.

I also updated the code of the interface test fixture in tests/conftest.py to directly delete the repository on an Artifactory backend at the end of the test without the need to first re-authenticate.

Benchmark code
import time

import audbackend


backend = audbackend.backend.Artifactory("https://audeering.jfrog.io/artifactory", "data-public")
backend.open()
interface = audbackend.interface.Maven(backend)

t0 = time.time()
interface.exists("/emodb", "1.4.1")
interface.exists("/emodb/db", "1.4.1")
interface.exists("/emodb/meta", "1.4.1")
interface.exists("/emodb/media", "1.4.1")
t = time.time()
print(f"{t - t0:.3f} s")

image

* Add audbackend.backend.Artifactory.path()

Before we had audbackend.backend.Artifactory._path(), which was returning an artifactory.ArtifactoryPath object that can be used to interact with the path on the backend. As this might also be helpful for a user (compare audeering/audb#386), I decided to change it into a public method.

image

@hagenw hagenw marked this pull request as draft April 10, 2024 13:31
@hagenw hagenw force-pushed the artifactory-authentication branch from a43e3da to 683079b Compare April 17, 2024 09:57
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.0%. Comparing base (215a098) to head (a38aba1).

Additional details and impacted files
Files Coverage Δ
audbackend/core/api.py 100.0% <100.0%> (ø)
audbackend/core/backend/artifactory.py 100.0% <100.0%> (ø)
audbackend/core/backend/base.py 100.0% <100.0%> (ø)

@hagenw hagenw marked this pull request as ready for review April 17, 2024 13:38
@hagenw hagenw changed the title Add audbackend.backend.Artifactory.authentication() Better authentication handling in backends Apr 17, 2024
@hagenw hagenw requested a review from frankenjoe April 17, 2024 13:42
@hagenw hagenw marked this pull request as draft April 18, 2024 08:09
@hagenw
Copy link
Member Author

hagenw commented Apr 18, 2024

I converted back to draft, as I would first like to solve one remaining problems with the tests.

@hagenw hagenw marked this pull request as ready for review April 18, 2024 08:23
@hagenw
Copy link
Member Author

hagenw commented Apr 18, 2024

I was able to fix the test related problems, and the pull request is again ready for review.

@hagenw
Copy link
Member Author

hagenw commented Apr 18, 2024

We could also speed up the tests on the Artifactory backend even further by just open one connection to the server and reuse it, but I would propose to not handle this in this pull request and created #218 to track it.

@frankenjoe
Copy link
Collaborator

audbackend.backend.Artifactory now only applies the authentication when calling open(). Before it did the authentication every time a path was requested on the backend, e.g. every time backend.exists(path) was called. Now, we store the path to the repo in self._repo, when calling open(), and can create all other paths based on top of that.

When asking if 4 files exists on the backend this reduced the execution time for me from 0.872 s to 0.178 s.

This is a great improvement and should lead to a significant speed up in audb when publishing / loading databases with many small files.

@hagenw
Copy link
Member Author

hagenw commented Apr 19, 2024

Yes, this would also be my hope, maybe I should try to benchmark that already.

@frankenjoe
Copy link
Collaborator

In audbackend.backend.FileSystem I then do not add auth as an argument to __init__(). Don't know if it would be better to always have it, but from a user perspective I found it easier this way.

Seems ok to me, to only add it to classes where it is actually needed.

@hagenw hagenw force-pushed the artifactory-authentication branch from 86624dd to a38aba1 Compare April 19, 2024 11:57
@frankenjoe frankenjoe merged commit b837d3e into dev Apr 19, 2024
9 checks passed
@frankenjoe frankenjoe deleted the artifactory-authentication branch April 19, 2024 16:17
hagenw added a commit that referenced this pull request May 3, 2024
hagenw added a commit that referenced this pull request May 3, 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