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

Add type to Broker's _actor_infos #205

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Impelon
Copy link
Contributor

@Impelon Impelon commented Apr 29, 2024

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
Copy link
Contributor Author

Impelon commented May 1, 2024

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
Copy link
Contributor Author

Impelon commented May 22, 2024

Same here, fixed the conflict.

@marvin-steinke
Copy link
Contributor

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.

@Impelon
Copy link
Contributor Author

Impelon commented Jul 22, 2024

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 :)

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.

Facilitate getting actor type in SIL interface
3 participants