Shall we allow an observer of type `functools.partialmethod`?

[sed-i]

I wrote a generic observer that looks like this:

    def _update_stored_event_tracker(self, key: str, value: bool, _event):
        self._stored[key] = value

and I'm trying to use it like this (context):

        # We use stored state to track the presence of a relation. "Joined" means we have it,
        # "broken" means we lost it. Use a generic observer to apply to all relations.
        # This is needed for large deployments to avoid long settling times incurred by multiple
        # calls to relation-list and relation-get.
        for rel_name, stored_key in relation_name_to_stored_key.items():
            self.framework.observe(
                self.on[rel_name].relation_joined,
                partialmethod(self._update_stored_event_tracker, key=stored_key, value=True),
            )
            self.framework.observe(
                self.on[rel_name].relation_broken,
                partialmethod(self._update_stored_event_tracker, key=stored_key, value=False),
            )

However, using partialmethod gives a type error:

https://github.com/canonical/operator/blob/89ea7f4d56173ea314d59a271e4304b18cd129ed/ops/framework.py#L790-L793

TypeError: Framework.observe requires a method as the 'observer' parameter, got functools.partialmethod(<bound method COSProxyCharm._update_stored_event_tracker of <ops.testing.Harness.begin.<locals>.TestCharm object at 0x7ff1ebb588e0>>, , key='have_gagent', value=True)

Related: https://github.com/canonical/operator/issues/545

[benhoyt]

See also https://github.com/canonical/operator/issues/1129, where we considered adding support for lambda functions (which don't work either). We decided not to at that time, primarily because, as Tony said:

Having the handlers be (methods on) an Object is core to how the framework handles emitting (and deferring) so that doesn't seem like something we can easily change.

I think this partialmethod approach would have the same issue.

I think -- given the context in your PR -- the straight-forward methods that just set self._stored.have_prometheus_rules = True or whatever are clearer than this kind of abstraction.

I was going to ask why you can't just get the relation info, as storing this in stored state seems like a recipe for getting out of sync. But then I saw your comment about how for large deployments all the relation-list and relation-gets are too slow. I presume you concluded that by measuring? I wonder if those hook tools are being called multiple times for the same relations, and we could solve that issue by caching instead?

[sed-i]

I presume you concluded that by measuring?

Not personally, but there's some related history.

[tonyandrewmeyer]

I think this would be easier than lambda, because you can do func.func (first func is the argument, second is the partialmethod attribute) to get the underlying function and then func.func.__self__ is the Object subclass. However, we need to have a name so that we can do (roughly) getattr(object_subclass, name)(event) in the emitting, and there's no name here.

However, if it was something like:

        for rel_name, stored_key in relation_name_to_stored_key.items():
            setattr(self, f"{rel_name}_{stored_key}_true", partialmethod(self._update_stored_event_tracker, key=stored_key, value=True))
            self.framework.observe(self.on[rel_name].relation_joined, getattr(self, f"{rel_name}_{stored_key}_true"))

Then I think it would likely work, if we changed observe to know about functools.partialmethod. ie. I think the unsolvable (without huge work) problem is the lack of a name, and the solvable (if merited) problem is that it's not a normal function.

[sed-i]

Thanks @tonyandrewmeyer. Your example seems to support the "don't over complicate observers" argument :D and I agree. Is deferring going to be deprecated? Would it matter for the "no name" problem?

[tonyandrewmeyer]

Your example seems to support the "don't over complicate observers" argument :D and I agree. Is deferring going to be deprecated? Would it matter for the "no name" problem?

Right now, all events go through the same emitting mechanism, deferred or not, so it wouldn't make a difference. I don't know if the whole storage notices system is there solely to handle deferred events or not (probably not? probably supports running multiple events from Juju in the same Charm object?), but changing the framework to actually call the provided observer rather than adding a notice to the storage and that getting pulled out again would be a pretty significant change (at least in that it would impact every single event, but probably in the amount of code and tests too).

[sed-i]

Ok. I had the impression that storage is needed only to be able to restore in defer. In any case, closing :) Thank you!

[tonyandrewmeyer]

Right now, all events go through the same emitting mechanism, deferred or not

Wrong! Sorry, I was remembering this incorrectly, and only noticed that today while I was looking at the relevant code for another reason.

We do still do a getattr to get the method by name rather than passing around the actual method object, but it would be less work to make it possible for the observer to not have a name if we didn't need to worry about deferred events.

[dimaqq]

Is deferring going to be deprecated?

Juju would have to gain deferral or retry mechanism. There was casual talk about that, but my understanding is that it's too early for Juju folks to plan that.

I have a feeling that there's a certain pattern in the overall ecosystem development:

• a great idea A is tried in a charm
• somewhat validated idea may get moved to a charm lib
• properly validated idea may become part of ops
• a validated and well-used idea may become a juju feature

Please mention that ops-side deferral prevented you from doing X to juju folks to add some impetus 🙇🏻

[tonyandrewmeyer]

Juju would have to gain deferral or retry mechanism. There was casual talk about that, but my understanding is that it's too early for Juju folks to plan that.

It's on the charm-tech roadmap for this cycle :smile:. It won't make it into 3.6 unless that gets massively delayed because the change-updated events were higher priority. I'm not sure when 4.0b1 is expected, but I have the feeling I might miss that too. If b1 is the cutoff for new features then it would end up landing in 4.1, but if features can be added during the beta cycle then 4.0 should be possible. (Assuming, of course, that we can come up with a spec that gets approved).

I have a feeling that there's a certain pattern in the overall ecosystem development:

• a great idea A is tried in a charm
• somewhat validated idea may get moved to a charm lib
• properly validated idea may become part of ops
• a validated and well-used idea may become a juju feature

I think the pattern we want is not quite this. If there's something that would potentially be a good fit for ops, then it's often initially done in charms, then as a lib, then in ops. If there's something that's potentially good in Juju, it may not be feasible at all outside of Juju, but if it is then it could start in charms and then become a lib, but it does not belong in ops (defer being a good example of something that probably should not have been added to ops).