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

Update to DynamicSumTypes 3 #1055

Merged
merged 56 commits into from
Aug 6, 2024
Merged

Update to DynamicSumTypes 3 #1055

merged 56 commits into from
Aug 6, 2024

Conversation

Tortar
Copy link
Member

@Tortar Tortar commented Jul 7, 2024

DynamicSumTypes v3 makes it really really easy to work with sum types directly, with much more flexibility than before and a much cleaner approach, we need to deprecate @multiagent because it was backed by the previous approach. This time I think we can go instead by integration, listing DynamicSumTypes in the "Performance Tips" of the manual and giving an example because we actually need almost no work here for that.

This will also fix #1039

@Datseris
Copy link
Member

Datseris commented Jul 7, 2024

we need to deprecate @multiagent

What?! You need to provide much more information for such a huge change. This is one of the big new things in v6 and you want to deprecate it in 6.1. This is not good experience for a user.

@Datseris
Copy link
Member

Datseris commented Jul 7, 2024

I don't understand this PR at all. @agent macro is completely unaffected and this PR replaces everything with @agent. How can this possibly not lead to the well known type instability performance problems we had pre v6?

@Tortar
Copy link
Member Author

Tortar commented Jul 7, 2024

This is still totally incomplete, I will add a lot more info when I finish the transition. In practice though we can replace all the work of @multiagent with

using Agents, DynamicSumTypes
# how many agents you want
@agent struct FirstAgent end
@agent struct SecondAgent end
# the sum type which removes all type instabilities available in DynamicSumTypes
@sumtype BothAgent(FirstAgent, SecondAgent) <: AbstractAgent

No need for a special macro for dispatch, no need to if-else branches. The sum type is actually made of real agents, and not by opaque representation like in @multiagent. Almost only plain Julia code.

@Tortar
Copy link
Member Author

Tortar commented Jul 7, 2024

If you take a brief look at the ReadMe repo you will see how simple the new methodology is: https://github.com/JuliaDynamics/DynamicSumTypes.jl

@Datseris
Copy link
Member

Datseris commented Jul 7, 2024

okay, when you update this please also write out, at least in brief, what was the new "magic" you discovered that made this possible at an internal level, as I am not sure I can figure it out by source diving.

@Tortar Tortar marked this pull request as ready for review July 7, 2024 16:38
@Tortar
Copy link
Member Author

Tortar commented Jul 7, 2024

In practice the "magic" is just to enclose all the types in a single one and then use if-else branches to type-stabilize the result of the various functions one needs to operate on them. I somehow overlooked this simple methodology initially because I didn't expect that something this simple could work that well considering the complexity of other packages trying to do something similar. I tested the methodology from 1.9 on with our benchmarks and it always removes any dynamic dispatch there was with a Union, e.g look at https://github.com/JuliaDynamics/Agents.jl/pull/1055/files#diff-7f4ca59230554e8d12359a1e1e1a3b4bc6accee721ebda1f9b8742814b231d29R156. And it is incredibly simple, like 50 locs of core code.

There is actually no need to go further than this in my opinion, Julia is good enough to do all the rest for us 👍

@Tortar Tortar requested a review from Datseris July 7, 2024 22:38
Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

I am stopping the review after reading through the tutorial. I've added lots of comments. Unfortunately @Tortar, I don't see this as an upgrade of what we had before. I don't see it as a downgrade either.

Please try to explain how this is a better version of what was happening with the @multiagent. Please keep in mind: we dont care how complicated the source code of DynamicSumTypes.jl is. We care about how complicated it is to use Agents.jl. My point being that this is a PR into Agents.jl, which is a rather front-end package. So the value of PRs here is mainly value for an end-user. That is why having e.g. sorter source code in DynamicSumTypes.jl is not an argument in favour of this PR by itself.

To my understanding, the main advantage over using @multiagent is that 1) @multiagent can lead to a rather large chunk of code, because all subagent types must be inside, while with this new approach the agent types are still kept separately. More over in this approach existing agent types can be reused. However, is this really such a big advantage? Agent types can only defined once regardless, as Julia can't redefine types. So does it matter if the agent declaration is inside @multiagent or not? With this new approach, you can also either use MultiAgent or Union. But does this really matter? As far as I can tell, one almost never has a reason to use Union, as the multi agent infrastructure is so good.

Now, here are some points where I don't see any advantage:

  • We replaced @multiagent with creating the other wrapper type MultiAgent. So we don't really gain anything here.
  • We didn't get rid of needing a replacement for typeof. We went from kindof to typeof(variant. This is not an improvement and I would argue it is more complex over kindof.
  • We didn't get rid of now having to alter the "standard Julia multiple dispatch" for the agent stepping function. We went from the @dispatch usage to the 3-argument agent_step! function, where one of the three input arguments isn't even used anywhere in the function. It is just used for dispatch. In my eyes this is a downgrade, because it is increase in complexity, without any obvious benefit. I like more the @dispatch and the 2-argument agent step, because it is more harmonious with the rest of the library.
  • We have a clear loss of benefit as now you can't use add_agent! the typical way. In 99% of the cases I've used Agents.jl, I was adding agents to the model via the add_agent! automatic route, I wasn't first creating them and then adding them to the model.

If you think this new approach is a better route for Agents.jl, then please make it transparent why you think so. I am really sorry for the negative review, as you've put in some work on this. But it is important that such a big change overcomes sufficient adversity before getting in.


Main comment 2: it is totally fine for you to reference DynamicSumTypes.jl, but the typical user of Agents.jl does not need to hear about this package, or even using it in their code. Instead, you can mention this infrastructure at the end of the docstring of @sumtype or the wrapper macro for it with a more Agents.jl like name.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
docs/src/performance_tips.md Outdated Show resolved Hide resolved
docs/src/tutorial.jl Outdated Show resolved Hide resolved
docs/src/tutorial.jl Show resolved Hide resolved
docs/src/tutorial.jl Outdated Show resolved Hide resolved
docs/src/tutorial.jl Outdated Show resolved Hide resolved
docs/src/tutorial.jl Outdated Show resolved Hide resolved
docs/src/tutorial.jl Show resolved Hide resolved
@Datseris
Copy link
Member

Datseris commented Jul 8, 2024

I tested the methodology from 1.9 on with our benchmarks and it always removes any dynamic dispatch there was with a Union

Right, but did you test if there is any performance regression with respect to using the current @multiagent approach? That is the important thing to check. If the code now became slower or not.

@Tortar
Copy link
Member Author

Tortar commented Jul 9, 2024

thanks for the review @Datseris, it is indeed a big change, but as I will explain really worthwhile in my opinion.

The problematic aspects of @multiagent which the new approach solves are:

  • You can't reuse types. You already noticed this, but underestimate its importance in my opinion. This is a very good feature, because let's say you want to reuse an already defined type in another library. This allows to do that, @multiagent does not because the representation is opaque.

  • The multiple dispatch it offers is incomplete and not totally correct. You can't for dispatch on

    • the @multiagent type and at the same time variants of a type. This is a fundamental limitation of the previous approach, because you would need to write ambiguity resolution rules by yourself (very very hard) and at the same time you would need to anyway through when ambigous while Julia just warns you.

    • Vararg arguments can sometime (rarely actually) cause incorrect results, this could be solved but it would require a fair bit of work.

  • You can actually create containers of only variants if you want, I'm not totally sure that this is very needed feature but for example you could do Agent1[variant(a) for a in agents(model) if variant(a) isa Agent1]. The previous approach is too opaque for this, you would end up breaking a lot of assumptions.

In practice the new approach is really the same of a Union of types but type stable...it is just a bit less ergonomic, but the way it works is very Julian in my opinion. It is easy to understand how to work with that, after some initial effort at least.

From the performance standspoint is actually amazing in my opinion. It is faster and more memory efficient than :opt_memory and only slower than :opt_speed when the types are very similar one from the other. This is because :opt_speed starts to lose its speed advantage when types becomes heterogeneous because they become fat memory wise which has effects also on the speed of them.

@Datseris
Copy link
Member

I think the tutorial should say how to add agents of a certain type in the model via add_agent! for both multi agent routes (which is teh recommended way to create new agents), rather how to just create a new agent and do nothing with it.

docs/src/tutorial.jl Outdated Show resolved Hide resolved
@Datseris
Copy link
Member

Datseris commented Jul 30, 2024

Thanks for finishing this @Tortar . Fine we can leave things as they are for the 3 argument version. I personally like the 2 argument only existing but oh well.

Let's try to get this in soon as I've seen several people use @multiagent already, so let's hope they won't complain too much :D

@Tortar
Copy link
Member Author

Tortar commented Jul 30, 2024

yes let's hope that this is the case, unfortunately this was a bit of unknown territory for me so I made some overly complex implementation at the start :-)

I addressed your review, if you approve I will do a final check and then merge

docs/src/tutorial.jl Outdated Show resolved Hide resolved
@Tortar Tortar changed the title Update to DynamicSumTypes 3: deprecate @multiagent Update to DynamicSumTypes 3 Aug 6, 2024
@Tortar Tortar requested a review from Datseris August 6, 2024 10:01
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Tortar and others added 2 commits August 6, 2024 12:05
Co-authored-by: George Datseris <[email protected]>
Co-authored-by: George Datseris <[email protected]>
Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

We are almost there, really. Only minor stuff and the event que model!

src/core/agents.jl Outdated Show resolved Hide resolved
src/core/agents.jl Outdated Show resolved Hide resolved
examples/event_rock_paper_scissors.jl Outdated Show resolved Hide resolved
@Datseris
Copy link
Member

Datseris commented Aug 6, 2024

Damn, using the multiagent instead of Union makes the event que example significantly more complicated.

@Tortar
Copy link
Member Author

Tortar commented Aug 6, 2024

not too much in my opinion, it is also a bit faster both in 1.10 and 1.11, 500ns per event instead of 560, not much but something :D

Copy link
Member

@Datseris Datseris left a comment

Choose a reason for hiding this comment

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

WIll merge when docs build.

@Datseris Datseris merged commit 6c98464 into main Aug 6, 2024
6 checks passed
@Datseris Datseris deleted the rem branch August 6, 2024 21:25
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.

Does EventQueueABM require @multiagent?
2 participants