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

feat(core): add capability to define persistent functionality to a dialogue edge #310

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

Archento
Copy link
Member

@Archento Archento commented Apr 9, 2024

By setting the persist property to True the defined default behaviour of an edge will not be overwritten when a developer uses the designated decorator.

Example in dialogue definition file:

async def persisting_function(ctx: Context, sender: str, msg: Type[Model]):
    ctx.logger.info("I was not overwritten, hehe.")
dialogue_edge.set_default_behaviour(MessageModel, persisting_function, persist=True)

Screenshot 2024-04-09 at 11 47 21

@Archento Archento requested review from Dacksus and jrriehl April 9, 2024 09:51
@Archento Archento closed this Apr 9, 2024
@Archento
Copy link
Member Author

Archento commented Apr 9, 2024

closing and reopening the PR to try to remove the expected Python3.8 check.

@Archento Archento reopened this Apr 9, 2024
fix: _func type declaration
Copy link
Contributor

@jrriehl jrriehl left a comment

Choose a reason for hiding this comment

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

Would you mind giving a bit more context / motivation on why this is needed? We are effectively registering multiple handlers for a dialogue edge here, which we decided not to support in message handlers for protocols. So with this there should be a particular reason why this needs to be supported in Dialogues and not in Protocols. And also, why limit to only 2 handlers for a given edge and not more?

@Archento
Copy link
Member Author

Archento commented Apr 9, 2024

Would you mind giving a bit more context / motivation on why this is needed? We are effectively registering multiple handlers for a dialogue edge here, which we decided not to support in message handlers for protocols. So with this there should be a particular reason why this needs to be supported in Dialogues and not in Protocols. And also, why limit to only 2 handlers for a given edge and not more?

Sure! Just to recap what has happened so far:
The first change to the dialogues in the last PR opened a way for the graph designer to pre-define behaviour per edge which is convenient and allows the abstraction of interactions from the user.
Screenshot 2024-04-09 at 15 06 00
In this example the dialogue consists of 5 interactions / messages, attached to edges. It is already possible to 'automate' the two interactions in the box and give them pre-defined MessageCallbacks so that the user essentially does not need to worry about these steps (and only needs to use 3 of the 5 potential decorators).
But if the user wants he can overwrite these predefined functions with his/her own functions by using the assigned decorators. Since we have no way of validating the contents of the functions (and don't want to) we cannot assure that dialogues which need to trigger certain behaviour perform like we expect them to do.

Now this PR is meant to provide a graph designer with the means to explicitly set default behaviour that is pre-attached to an edge but which cannot be overwritten by the user. It is an explicit "just one function added to the interaction" to keep the number of dialogue exchanges small but to trigger essential behaviour which is needed for the dialogue. It should be seen more as: there is an interaction happening but a user/dev/company is able to provide additional functionality on top of what needs to happen. (similar to attaching additional metadata).

We need something like this for the Exchequer dialogue where certain requests need to happen to the backend but the user would be able to additionally inform the counterparty or attach business specific logic to the interactions.
Regarding the core framework: I would also like to keep it to just one handler per Message so this here should be seen as an exception to the rule.

Dacksus
Dacksus previously approved these changes Apr 9, 2024
@jrriehl
Copy link
Contributor

jrriehl commented Apr 9, 2024

Thanks for adding the context, that definitely helps a lot! I might still not be fully understanding it, but I have a small concern that we are conflating 2 separate concepts here that I think of as an "edge handler" and a "message handler". The edge handler is something that should happen every time the state transition occurs regardless of how the Dialogue is instantiated (what specific message is sent), and the message handler is something that should happen whenever a particular message activates the state transition. We have already added the ability to set a default message handler, which can be overwritten by an instance of the Dialogue, and it sounds like this PR is about adding the ability to add a default edge handler, which is not meant to be overwritten by an instance of the Dialogue. Is that a reasonable way to think about this?

@Dacksus
Copy link
Contributor

Dacksus commented Apr 9, 2024

Would you mind giving a bit more context / motivation on why this is needed? We are effectively registering multiple handlers for a dialogue edge here, which we decided not to support in message handlers for protocols. So with this there should be a particular reason why this needs to be supported in Dialogues and not in Protocols. And also, why limit to only 2 handlers for a given edge and not more?

@jrriehl re: why only 2 handlers:
Informally, dialogues have 2 levels of abstraction: the 'pattern' defining the generally possible sequences of messages and the 'dialogue' which is instantiates a pattern by adding actual message models (although this now gets weakened in some sense, since this PR introduces message models already on the pattern level). This abstraction considers that patterns and dialogues might be defined and used by 2 distinct roles. For now it made sense to enable adding one handler per step/role and not support / encourage even more indirection layers.

@jrriehl
Copy link
Contributor

jrriehl commented Apr 9, 2024

@jrriehl re: why only 2 handlers: Informally, dialogues have 2 levels of abstraction: the 'pattern' defining the generally possible sequences of messages and the 'dialogue' which is instantiates a pattern by adding actual message models (although this now gets weakened in some sense, since this PR introduces message models already on the pattern level). This abstraction considers that patterns and dialogues might be defined and used by 2 distinct roles. For now it made sense to enable adding one handler per step/role and not support / encourage even more indirection layers.

Yes, this is where I ended up too, but it took a bit of time to both understand what is going on conceptually and to convince myself that this implementation did what was intended. That's why I think there might be some room for improvement in the implementation.

Copy link
Contributor

@jrriehl jrriehl left a comment

Choose a reason for hiding this comment

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

Nice, very clear now.

@Archento Archento merged commit a5d519a into main Apr 11, 2024
9 checks passed
@Archento Archento deleted the feat/dialogues-complex-default-behaviour branch April 11, 2024 14:00
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.

3 participants