canonical / data-platform-libs

A collection of charm libraries curated by the Data Platform Team
https://charmhub.io/data-platform-libs
Apache License 2.0
10 stars 9 forks source link

`TypeError: metaclass conflict` encountered when using data_interfaces charm library #45

Closed NucciTheBoss closed 1 year ago

NucciTheBoss commented 1 year ago

Bug Description

My charm slurmdbd-operator fails to install because the error TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases is thrown when importing the __data_interfaces__ charm library.

Looking through the charm library, the issue is being caused by the DataProvides and DataRequires base classes trying to inherit from both ops.framework.Object and abc.ABC. ops.framework.Object and abc.ABC use different metaclasses so conflicts are encountered when trying to define DataProvides and DataRequires at charm runtime.

To Reproduce

  1. git clone git@github.com:NucciTheBoss/slurmdbd-operator.git@mysql-migration
  2. cd slurmdbd-operator && charmcraft -v pack
  3. juju deploy ./slurmdbd_ubuntu-20.04-amd64_centos-7-amd64.charm slurmdbd
  4. See error once install hook is executed.

Environment

I am running Juju locally using my LXD server hosted on my workstation. I am using v0.6 of the __datainterfaces charm library. I am using Juju version 3.1 and my slurmdbd-operator is current using Ubuntu 20.04 LTS as its base.

Relevant log output

unit-slurmdbd-1: 14:57:56 INFO juju.worker.uniter found queued "install" hook                                                                                                                                      
unit-slurmdbd-1: 14:57:56 WARNING unit.slurmdbd/1.install Traceback (most recent call last):                                                                                                                       
unit-slurmdbd-1: 14:57:56 WARNING unit.slurmdbd/1.install   File "./src/charm.py", line 12, in <module>                                                                                                            
unit-slurmdbd-1: 14:57:56 WARNING unit.slurmdbd/1.install     from charms.data_platform_libs.v0.data_interfaces import (                                                                                           
unit-slurmdbd-1: 14:57:56 WARNING unit.slurmdbd/1.install   File "/var/lib/juju/agents/unit-slurmdbd-1/charm/lib/charms/data_platform_libs/v0/data_interfaces.py", line 354, in <module>                           
unit-slurmdbd-1: 14:57:56 WARNING unit.slurmdbd/1.install     class DataProvides(Object, ABC):                                                                                                                     
unit-slurmdbd-1: 14:57:56 WARNING unit.slurmdbd/1.install TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases                      
unit-slurmdbd-1: 14:57:56 ERROR juju.worker.uniter.operation hook "install" (via hook dispatching script: dispatch) failed: exit status 1                                                                          
unit-slurmdbd-1: 14:57:56 INFO juju.worker.uniter awaiting error resolution for "install" hook

Additional context

To resolve the metaclass conflicts, you need to create a new metaclass that inherits from both ops.framework._Metaclass and abc.ABCMeta.

NucciTheBoss commented 1 year ago

So upon further investigation it seems that the metaclass conflict is not an issue in ops >= 2.0.0 but an issue in ops <= 1.5.4. This is because in ops==2.0.0, ops.framework.Object does not inherit from _Metadata as its metaclass. The __data_interfaces__ charm library will work with charms that use ops 2 or greater, but not charms that use an ops version below that.

There are two potential approaches that could be used to address this issue:

  1. Include a note in the documentation for __data_interfaces__ that it is not compatible with ops version < 2.0.0. In my case I can just update to ops==2.0.0, but some folks might not be able to do that so easily.
  2. Use some fancy Python logic to create a composed metaclass for DataProvides DataRequires if the charm is using a ops version < 2.0.0.
deusebio commented 1 year ago

Hi @NucciTheBoss ! Thanks for this.

Yes, the data_interface library starting from patch version 3 is not backward compatible with ops<2.0. Indeed, that patch upgrade was done in order to comply with ops>=2.0 , here is the PR. In older version of 1.5.4, Object class of ops had its own Metaclass, which was removed in ops>=2.0. Prior to 2.0, in order to create the abstractions in the data_interface we therefore had to mix metaclasses (otherwise you would get the error of the metaclass conflicts), thus importing the ops metaclass _Metaclass that was later removed from ops.

Therefore:

I understand that this breaks backward and forward compatibility but we decided to NOT address this since up to patch 2 it was a fairly new library (edge) with little user base, which did not warrant supporting either versions or upgrading the major of the charm library. In general I believe we should try to switch to using ops 2.0 in general, and this upgrade (ops from 1 to 2 major) should be rather painless. But if you can't upgrade ops to 2.0, you may temporary fix this by cherry-picking the modification in PR #35 . But let us know (on mattermost) if this is the case so we have a sense how many people are still struggling with this (considering whether or not placing some fancy python logic).

deusebio commented 1 year ago

Ah, by the way, following up this comment, I'll be raising a PR where:

  1. We update the documentation as you have suggested
  2. We also introduce the dependency using PYDEPS in the charm lib
NucciTheBoss commented 1 year ago

Ah, by the way, following up this comment, I'll be raising a PR where:

  1. We update the documentation as you have suggested
  2. We also introduce the dependency using PYDEPS in the charm lib

This would be great. Once you make that pull request updating the documentation, you can link this issue to close once the pull request is merged. As long as there is a notice to use ops 2.x in the documentation with the declared PYDEP, I am more than happy to have this issue marked as resolved!