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

Depend on audbackend>=2.0.0 #386

Merged
merged 2 commits into from
May 10, 2024
Merged

Depend on audbackend>=2.0.0 #386

merged 2 commits into from
May 10, 2024

Conversation

hagenw
Copy link
Member

@hagenw hagenw commented Apr 10, 2024

Updates the code to use the new API of audbackend 2.0.0.

The main changes in audbackend 2.0.0 affecting audb are:

  • The new abstraction layer of interfaces is introduced. A backend object is now only responsible for accessing the backend. The interface is responsible for managing how the files are stored on the backend, e.g. versioning them by storing them in a correspoinding folder structure. For audb we use the audbackend.interface.Maven for versioning on the audbackend.backend.Artifactory backend.
  • Authentication on the backend is only done when explicitly opening a connection to a backend, using audbackend.backend.Base.open() or using the with statement. This also means it is desired to use audbackend.backend.Base.close() when the backend is no longer needed.
  • audbackend.Repository was removed as audbackend is no longer using string names like artifactory to address a backend. As we still use names in audb to address a backend, e.g. in the config file, we need to handle this here directly.
    To do so, it implements now its own version of audb.Repository and does no longer rely on audbackend.Repository, so we can deprecate that as well.

image

@hagenw
Copy link
Member Author

hagenw commented May 2, 2024

@ChristianGeng as I would like to release audbackend 2.0.0 next week, it would be great if you could review this here already, so we can make a new release of audb very soon after audbackend 2.0.0 is released.

@ChristianGeng
Copy link
Member

@ChristianGeng as I would like to release audbackend 2.0.0 next week, it would be great if you could review this here already, so we can make a new release of audb very soon after audbackend 2.0.0 is released.

My understanding is that the current main branch of audbackend is the release 2.0.0 candidate, right?

@hagenw
Copy link
Member Author

hagenw commented May 3, 2024

@ChristianGeng as I would like to release audbackend 2.0.0 next week, it would be great if you could review this here already, so we can make a new release of audb very soon after audbackend 2.0.0 is released.

My understanding is that the current main branch of audbackend is the release 2.0.0 candidate, right?

No, the dev branch is the 2.0.0 candidate: https://github.com/audeering/audbackend/tree/dev.
This is just to be on the safe side, if we need to make an immediate bug fix release for audbackend 1.0.x, which can still be done from the main branch. Shortly before releasing audbackend 2.0.0, we will rename the current main branch to 1.0.x, and dev to main.

audb/core/api.py Outdated Show resolved Hide resolved
Copy link
Member

@ChristianGeng ChristianGeng left a comment

Choose a reason for hiding this comment

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

The Merge Requests makes use of new features in audbackend. The features themselves are useful and better the achitecture / design of the involved packages into the direction of more loosely coupled components.

  • The interfaces subpackage in audbackend
  • The backend subpackage in audbackend
  • The Repository object here in audb

This great advancement comes at a cost: audb and auadbackend are more complex to study when wanting to gain an understanding of the two packages under consideration and their interactions. The main points raised in the review targeted the process of getting to grips with this structure. These points were not so much about implementation details, rather more about the organization and terminology of the areforementioned building blocks and their documentation.

  • both the interfaces and the backend subpackages are implemented using oop inheritance. The abstract class is in both cases called Base declaring that it should not be instantiated. It would probably be more in accordance with general terminology to call these Backend in the case of `backen` and make them - in the absence of the abstract keyword in Python - an Abstract Base Clase.
  • the name interfaces is quite generic. Mainly this audbackend subpackage is about server-side directory layouts.
  • The Repository class maintains an inventory of backend classes, but on __call__ returns an interface associated with the backend class in the inventory. So its main purpose is object creation that looks what is very similar to what often is idiomatically implemented as a factory pattern.
  • I had proposed documenting creating and documenting type stubs, with high level documentation being more in the .pyi files - similar to C++. Now It think this is over the top. However, bird's eye documentation in addition to the new developer guide in audbackend would come in handy.
  • Concerning backend and interface, it might be that this alternatively could be implemented using multiple inheritance. Then the Artifactory backend would get its directory layout management from the Maven Interface. Multiple inheritance in Python is often flagged by calling the classes that implement the additional functionality Mixin. I am not sure right now whether this would fly, but this is a pattern that is quite often seen in Python projects.

Taken together: the architecture in audbackend-2.0 is clearly improved, this MR simply makes use of it. Imo there would be leeway of improving the process of getting other devs on board in audbackend. However I would - given that this will be needed in the future - postpone this for a later release. Therefore I am approving - as on this side there is not really much to do.

@hagenw hagenw marked this pull request as ready for review May 10, 2024 07:08
@hagenw hagenw merged commit 3693061 into main May 10, 2024
7 checks passed
@hagenw hagenw deleted the audbackend-2.0.0 branch May 10, 2024 07:13
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