vessim icon indicating copy to clipboard operation
vessim copied to clipboard

Add type to `Broker`'s `_actor_infos`

Open Impelon opened this issue 1 year ago • 2 comments

This is one of the solutions I suggested - fixes #203. I'm not sure it is the solution that is the best, because it changes the interface of the broker again. But it stores the type directly with the state, which I think is nice. I don't mind another solution being added instead of this one though!

Impelon avatar Apr 29 '24 12:04 Impelon

I'm going to sneak 275d37d in here. (So I do not need to hand in another pull-request after this one with an updated signature :) )

I think it would also be good to expose the actor infos directly, since otherwise it would be hard for the API to find out which actors are actually available.

Impelon avatar May 01 '24 19:05 Impelon

Same here, fixed the conflict.

Impelon avatar May 22 '24 10:05 Impelon

Hey :) so, we recently conducted a redesign of the Actor methodology. The regular Actor, which can also be seen in the example in the readme, now simply accepts a Signal. We think most Actor behaviors can be expressed this way. If not, the ActorBase ABC is to be sublcassed (Actor is also just an ActorBase implementation). Because we expect most Actors to be of this new type, I don't think adding a type to all actor infos/states is a common use case. For your usecase: Can you add the type to the actor state? (There will also be a new PR making this process a bit easier for the regular Actor, instead of overwriting the whole function that is)

I think it would also be good to expose the actor infos directly, since otherwise it would be hard for the API to find out which actors are actually available. Will do.

marvin-steinke avatar Jul 22 '24 11:07 marvin-steinke

For your usecase: Can you add the type to the actor state?

Hey :) I mainly used the type to be able to differentiate vessim's Generator actors from other actors in version 0.6.0. So I couldn't just add my own information into the pre-defined Generator actor and I did not want to force users to use a new actor class just to add this information. I initially expected to need the type information to differentiate a few more actor types from each other. But for this main use case, I could have just differentiated them according to the signedness of p in the actor state, which is still possible in newer vessim versions.

Like I said, a type field would also allow an API to know what kind of information to expect in the actor state. However it is true that Python's philosophy is duck typing anyway, so one could argue that one should just check whether the required information for an action is available in the actor state.

There will also be a new PR making this process [(adding the information to the actor state)] a bit easier for the regular Actor [...]

Right, that's not a bad feature actually. Then, my argument for adding the type would be completely pointless, as the type would not tell the API at all which information to expect from an actor state.

I'd say you can close this PR and #203 then, since they are not relevant to the current vessim version anymore. From the point of developing a custom API it would be nice to easily tell what kind of actor state one is dealing with, but I'm not sure is a good built-in solution for this with the new actor system.

I think it would also be good to expose the actor infos directly [...]

Will do.

Thanks :)

Impelon avatar Jul 22 '24 16:07 Impelon