Experiment with PoolableStatuses

[ghislainbourgeois]

Small experiment to work try and fix #24.

When assigning StatusBase objects to a StatusPool, we dynamically transform those object in a poolable version, by creating a new type that inherits from both Status and the concrete StatusBase type of the passed object.

In other words:

type(charm.status.relation_1)  # Status
isinstance(charm.status.relation_1, Status)  # True
isinstance(charm.status.relation_1, StatusBase)  # False

charm.status.relation_1 = ActiveStatus()

type(charm.status.relation_1)  # PoolableActiveStatus
isinstance(charm.status.relation_1, Status)  # True
isinstance(charm.status.relation_1, StatusBase)  # True
isinstance(charm.status.relation_1, ActiveStatus)  # True

There are also some changes to Status to complete the goal:

charm.status.relation_1 = ActiveStatus()

charm.status.relation_1 == ActiveStatus()  # True
ActiveStatus() == charm.status.relation_1  # True

This is only a work in progress to get some discussions started, the unit tests were still passing locally, but this probably breaks stuff.

[PietroPasotti]

It's unclear to see from the diff what changed precisely, because of the formatting, could you roll that part back?
I see however that you commented out Status.__set__ and __get__, why is that?

RE the dynamic class creation just to happify instance checks, it seems like a partial solution, because, suppose we merge this PR:

active_status = ActiveStatus('foo')
self.pool.a = active_status

assert self.pool.a == active_status  # passes
assert isinstance(self.pool.a, ActiveStatus)  # passes
assert self.pool.a is active_status  # fails!

So what are we trying to achieve?
I get your point that there is an expectation that if you assign a=b then you expect a==b to pass, but the more we try to make a behave just like b would, the more we approach an uncanny valley of "that's still weird though" IMHO.

I think we'd better take one of two radical approaches:

1. make it clear that that expectation ain't going to be met. Document it, give a better error message (override __eq__, if other is a StatusBase, explain why you can't compare them that way).

2. drop the assignment-driven, unit-status-set-style of doing things. Drop the descriptor approach. Follow PoC library simplification #6 in forcing the whole pool to be dynamically built up.

self.pool = Pool()
self.pool.add('a', Status())  # implicitly starts at Unknown, set later. 
# OR:
self.pool.add('a', ActiveStatus('foo'))  # initial value

self.pool.set('a', ActiveStatus('foo'))
status = self.pool.get('a')
ori_status = status.wrapped   # this **is** ActiveStatus('foo')

[ghislainbourgeois]

I rolled back the formatting, sorry about that, it is run automatically from my editor.

About __set__ and __get__, it was causing problems with the changes I made because we are assigning a new object, and with __set__ specifically it was keeping the original object. I tried it without those 2 methods and the tests were still passing.

I completely agree that it is not a perfect solution because the object that is ultimately assigned is not the same as the object that was provided. I did this experiment mostly to get a better understanding of what was possible and I do not think we should merge it ever.

Regarding the approaches you are suggesting, I do not think I agree with 1. We will basically making syntactic sugar to make assignments look like unit-status-set-style, but behave differently.

Regarding 2, I think it is more explicit and would prefer that option if we need to keep all of this in this library.

I would suggest an option 3. If we were to make the ops statuses Poolable, by adding some of the code from this project's Status to ops.model.StatusBase, we would be able to simplify this library a lot, and keeping the unit-status-set-style.

The only feature that would be difficult to keep as-is in the case is unset(), but we could use the pool for that feature.

[PietroPasotti]

Yeah, the long-term plan is to move some of this functionality to ops, so perhaps I've chosen the wrong approach with this library. Maybe what it should be doing is monkey-patch StatusBase to inject the Status functionality, to basically make ops behave how it would behave if we were to migrate this functionality to ops natively.

The only new object would then be a StatusPool and the user code would look like:

from charms... import compound_status
compound_status.install()

from ops import *


class MyCharm(CharmBase):
    def __init__(...):
        self.pool = compound_status.StatusPool(self, auto_commit:bool, clobberer=compound_status.Summary())
        self.pool.add('foo')
        self.pool.add('bar', ActiveStatus('hello charm', tag='foo'))
        
    def _on_event(...):
        self.pool.add('baz', WaitingStatus())
        self.pool.unset('bar')
        
        previous: ActiveStatus = self.pool.get('foo')
        previous.warning('this status is about to be nuked')
        
        self.pool.set('foo', ActiveStatus(str(previous)))
        self.pool.delete('bar')

We're getting pretty close to @swalladge 's proposal I believe.
@rwcarlsen also had a similar take, but his approach (to make pool dict-like with setitem/getitem/delitem) would still miss a magic method that would correspond to unset.

Unless, of course, since everything is now dynamic, we drop unset as we can simply delete stuff and add it back later.

🤔