search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
720 stars 38 forks source link

[REVIEW]: Mango.jl: A Julia-Based Multi-Agent Simulation Framework #7098

Closed editorialbot closed 3 weeks ago

editorialbot commented 2 months ago

Submitting author: !--author-handle-->@jsagerOffis<!--end-author-handle-- (Jens Sager) Repository: https://github.com/OFFIS-DAI/Mango.jl Branch with paper.md (empty if default branch): joss_paper Version: v0.4.0 Editor: !--editor-->@jbytecode<!--end-editor-- Reviewers: @aurorarossi, @Tortar Archive: 10.5281/zenodo.13860452

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/0a7712eccc9ba5ccb2276d4f19c8f439"><img src="https://joss.theoj.org/papers/0a7712eccc9ba5ccb2276d4f19c8f439/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/0a7712eccc9ba5ccb2276d4f19c8f439/status.svg)](https://joss.theoj.org/papers/0a7712eccc9ba5ccb2276d4f19c8f439)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@aurorarossi & @Tortar, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jbytecode know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Checklists

📝 Checklist for @Tortar

📝 Checklist for @aurorarossi

editorialbot commented 2 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 2 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.softx.2024.101791 is OK
- 10.1007/s41109-023-00553-8 is OK
- 10.1186/s42162-022-00209-4 is OK
- 10.1145/3447555.3465415 is OK
- 10.15439/2021F60 is OK
- 10.1016/j.arcontrol.2019.05.006 is OK
- 10.1007/s10462-021-09996-w is OK
- 10.1561/2600000019 is OK
- 10.1177/00375497211068820 is OK
- 10.1007/978-3-030-61255-9_30 is OK
- 10.1145/375735.376120 is OK
- 10.1007/0-387-26350-0_7 is OK
- 10.1137/141000671 is OK

MISSING DOIs

- No DOI given, and none found for title: Artificial intelligence a modern approach
- No DOI given, and none found for title: Netlogo: A simple environment for modeling complex...
- No DOI given, and none found for title: JIAC V: A MAS framework for industrial application...
- No DOI given, and none found for title: agentframework (2.0.1) [last access 07-08-2024]

INVALID DOIs

- None
editorialbot commented 2 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.04 s (1369.3 files/s, 149007.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           28            716            616           2692
Markdown                        16            400              0           1009
TeX                              1             17              0            177
YAML                             6             17              8            162
SVG                              2              0              0            130
TOML                             2              5              0             36
-------------------------------------------------------------------------------
SUM:                            55           1155            624           4206
-------------------------------------------------------------------------------

Commit count by author:

   130  Rico Schrage
    60  Jens Sager
     8  Jan-Hoerding
editorialbot commented 2 months ago

Paper file info:

📄 Wordcount for paper.md is 1081

✅ The paper includes a Statement of need section

editorialbot commented 2 months ago

License info:

✅ License found: MIT License (Valid open source OSI approved license)

editorialbot commented 2 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jbytecode commented 2 months ago

@aurorarossi, @Tortar - Dear reviewers, you can start with creating your task lists. In that list, there are several tasks.

Whenever you perform a task, you can check on the corresponding checkbox. Since the review process of JOSS is interactive, you can always interact with the author, the other reviewers, and the editor during the process. You can open issues and pull requests in the target repo. Please mention the url of this page in there so we can keep tracking what is going on out of our world.

Please create your tasklist by typing

@editorialbot generate my checklist

Thank you in advance.

Tortar commented 2 months ago

Review checklist for @Tortar

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Tortar commented 2 months ago

Hi @rcschrg @jsagerOffis !

I read the article and given that there is stated that

The main reason for this julia-based version is to allow better focus on simulation36
performance, enabling larger scales of multi-agent simulations

I think a good addition to the paper would be a benchmark between the Python and Julia version of the software. Do you already have some examples implemented in both frameworks to use for this?

jsagerOffis commented 2 months ago

Hi @Tortar !

We have did some benchmarks like that for the proof of concept version of Mango.jl. I had already planned to update those to the current version since a lot has changed since then, so I will get on that asap now.

aurorarossi commented 2 months ago

Review checklist for @aurorarossi

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

jsagerOffis commented 2 months ago

@Tortar I have just updated our old benchmark scripts to the new Mango.jl version and added a small performance section to the paper.

I have linked the benchmark repository in the paper and we should be able to set that one to be public as well shortly.

aurorarossi commented 2 months ago

Hello,

I’ve started reviewing the package, and I’ve noticed the following:

jsagerOffis commented 2 months ago

@aurorarossi Hi, thanks for the inputs.

API List and Docstring standards: we will get on this and then merge it back over from the development branch.

Examples:

Regarding the send_message bug: Going by the second suggested method candidate, it looks like you have a local definition of the AgentAddress struct so it can not resolve the correct method. I assume this happened by copying the struct info we put in the corresponding part of the documentation?

Otherwise the send_message call looks like it should function.

Tortar commented 2 months ago

@editorialbot generate pdf

editorialbot commented 2 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

aurorarossi commented 2 months ago

@jsagerOffis I tried again, copying and pasting the example from the paper. The send_message function works now, but it seems to be stuck in a loop. image

Could the issue be that both containers have the same name? Shouldn't each container/agent have a unique name? image

jsagerOffis commented 2 months ago

@aurorarossi Is it actually stuck? The output looks as it should be.

It's just 2 agents sending messages to each other without any delays so it creates a lot of prints quickly. The main loop sleeps for 5 seconds (could maybe reduce that time in the example) and it terminates fine for me after those seconds.

I tested it with both single and multi-threaded julia to ensure its not some weird race condition now.

Could the issue be that both containers have the same name? Shouldn't each container/agent have a unique name? Agent names are only unique (and set) within the container they are registered to. In this case we use 2 containers on two different addresses, with one agent each, and since we do not specify an agent name, they both default to the first automatically chosen name.

Instead, if we tried to set the same agent name within the same container, it would be altered and a warning should be logged.

jsagerOffis commented 2 months ago

I added a delay to the example agents answering so they spam less and also changed their names and outputs to make things a little clearer.

example_output

aurorarossi commented 2 months ago

@jsagerOffis, I've identified the issue. When I copy and paste the following block from your paper:

# Start the container
wait(Threads.@spawn start(container))
wait(Threads.@spawn start(container2))

# Send the first message to start the exchange
send_message(ping_agent, "Ping", address(pong_agent))

it runs in a loop because it lacks the second part, which is in a different markdown block:

# wait a bit to see some outputs
sleep(5)

@sync begin
    Threads.@spawn shutdown(container)
    Threads.@spawn shutdown(container2)
end

I strongly suggest creating a function that encapsulates both blocks of code.

jsagerOffis commented 2 months ago

I strongly suggest creating a function that encapsulates both blocks of code.

Great suggestion. @rcschrg is currently adding this with syntax like:

activate(containers) do
    # Send the first message to start the exchange
    send_message(ping_agent, "Ping", address(pong_agent))

    # wait a bit to see some outputs
    sleep(5)
end
jbytecode commented 2 months ago

@aurorarossi, @Tortar, @jsagerOffis - Hey dears, thank you for such a smooth reviewing and the prefect dialog, I always keep tracking the issues and you're doing it very well. Thank you in advance.

Tortar commented 2 months ago

I'm not sure that these are the same issues @aurorarossi found but I tried to run the example in the ReadMe but it errors at some point and then it runs in a loop (maybe)

julia> wait(send_message(ping_agent, "Ping", AgentAddress(aid=pong_agent.aid, address=InetAddr(ip"127.0.0.1", 2939))))

ERROR: UndefVarError: `AgentAddress` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
 [1] top-level scope
   @ REPL[16]:1

# this never finishes?
julia> wait(Threads.@spawn begin
           while ping_agent.counter < 5
               sleep(1)
           end
       end)
jsagerOffis commented 2 months ago

@Tortar Well, if the first part there errors, its unsurprising that the second part can not finish.

The example setup is:

wait(Threads.@spawn begin
           while ping_agent.counter < 5
               sleep(1)
           end
     end)

This waits for the ping agent to have received 5 mesages but since the initial message errored, this never happens. As for why the initial send message errors to begin with... I actually don't know.

All the examples will slightly change today once I reviewed the new feature @rcschrg pushed so I hope that will make the example simpler.

Until then I just made this small mash-up of the paper and README.md examples (which I will turn into the same example for consistency later). This does the same thing as the README.md example but with nicer outputs and with the same stopping logic. Just ran this and it worked as expected so please let me know if it causes an issue for you.

Note that if you paste this into the REPL piece by piece it will start printing messages indefinitely after send_message(ping_agent, "Ping", address(pong_agent)) but should still be responsive to the remaining commands to shut it down (because agent logic is threaded in the background).

using Mango
using Sockets: InetAddr, @ip_str

# Create the container instances with TCP protocol
container = Container()
container.protocol = TCPProtocol(address=InetAddr(ip"127.0.0.1", 5555))

container2 = Container()
container2.protocol = TCPProtocol(address=InetAddr(ip"127.0.0.1", 5556))

@agent struct TCPPingPongAgent
    counter::Int
end

# Create instances of ping pong agents
ping_agent = TCPPingPongAgent(0)
pong_agent = TCPPingPongAgent(0)

# register each agent to a container and give them a name
register(container, ping_agent, "Agent_1")
register(container2, pong_agent, "Agent_2")

import Mango.handle_message

# Override the default handle_message function for ping pong agents
function handle_message(agent::TCPPingPongAgent, message::Any, meta::Any)
    println(
        "$(agent.aid) got a message: $message." *
        "This is message number: $(agent.counter) for me!"
    )

    # doing very important work
    sleep(0.5)

    if message == "Ping"
        agent.counter += 1
        t = AgentAddress(meta["sender_id"], meta["sender_addr"], nothing)
        send_message(agent, "Pong", t)
    elseif message == "Pong"
        agent.counter += 1
        t = AgentAddress(meta["sender_id"], meta["sender_addr"], nothing)
        send_message(agent, "Ping", t)
    end
end

# Start the container
wait(Threads.@spawn start(container))
wait(Threads.@spawn start(container2))

# Send the first message to start the exchange
send_message(ping_agent, "Ping", address(pong_agent))

while ping_agent.counter < 5
        sleep(1)
end

@sync begin
    Threads.@spawn shutdown(container)
    Threads.@spawn shutdown(container2)
end
jsagerOffis commented 2 months ago

@Tortar Are you running the latest version of Mango.jl? AgentAddress on top level was only added in 0.3.0 iirc. We pushed that release prior to submitting the paper but maybe you got 0.2.x for some reason anyway?

Tortar commented 2 months ago

yes, sorry, I had 0.2.1 for some reason installed, but now I updated to 0.3.0 and everything works fine! sorry for not noticing

jsagerOffis commented 2 months ago

Hi @Tortar @aurorarossi we have now merged some major changes onto the joss_paper and development branches, including:

All of this will make its way to main as release 0.4.0 but we may want to resolve some of the new issues before that.

jsagerOffis commented 2 months ago

@editorialbot generate pdf

editorialbot commented 2 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

aurorarossi commented 1 month ago

Ok, now the example from the README.md works. I tried the examples in the documentation, in particular, in the Agents section, but it does not work (second block of Message handling). See: image

Please check that all the blocks of the docs work.

jsagerOffis commented 1 month ago

The intent of the example here was to highlight that (presumably) known functions from "non-role agents" function the same on roles which is why the function calls are just filled with dummy values and the agent is not registered to a container (because this is not a role-specific thing).

@aurorarossi Is it actually necessary for every individual code block to be executable on its own? In my opinion, making sure each block here runs standalone would needlessly obscure the intended information of this part of the documentation with boilerplate code.

jsagerOffis commented 1 month ago

We are currently trying to come up with an idea to make all code blocks executable while also saving a bit on boilerplate code for readability.

Also noted a mistake in the above example. second line should be MyRole of course:

agent = MyAgent("")
role = MyAgent("")
aurorarossi commented 1 month ago

@aurorarossi Is it actually necessary for every individual code block to be executable on its own? In my opinion, making sure each block here runs standalone would needlessly obscure the intended information of this part of the documentation with boilerplate code.

I run the above blocks as well. I agree that the single block should not run standalone, but with the above run blocks everything should work. Otherwise I think these examples are more confusing than helpful. But I think this is something standard at least for the Julia community.

jsagerOffis commented 1 month ago

@aurorarossi Just merged the new changes into development and joss_paper. All examples should now be properly executable.

aurorarossi commented 1 month ago

Ok I checked the examples and they work. I also made a pull request to fix some typos and improve the flow of some sentences.

aurorarossi commented 1 month ago

Hi @jbytecode I am satisfied with the revisions.

jbytecode commented 1 month ago

@aurorarossi - Thank you so much for finalizing your review.

@Tortar - May I have a status update, if possible? Thank you in advance.

Tortar commented 1 month ago

Hy @jbytecode sorry for the delay, I'm happy with all of it now. Great work!

Just one small final detail: given that there is some code in the paper, I think it could be useful to mention the version of Mango.jl with which it has been executed for easier future reproduction

jbytecode commented 1 month ago

@Tortar - Thank you for finishing your review.

@jsagerOffis - Could you please address the latest changes and then ping me?

jsagerOffis commented 4 weeks ago

@jbytecode I just pushed a line giving the version to the paper. The changes made during the review will be their own new version and also receive a tagged branch.

jbytecode commented 4 weeks ago

@editorialbot check references

editorialbot commented 4 weeks ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

✅ OK DOIs

- 10.1016/j.softx.2024.101791 is OK
- 10.1007/s41109-023-00553-8 is OK
- 10.1186/s42162-022-00209-4 is OK
- 10.1145/3447555.3465415 is OK
- 10.15439/2021F60 is OK
- 10.1016/j.arcontrol.2019.05.006 is OK
- 10.1007/s10462-021-09996-w is OK
- 10.1561/2600000019 is OK
- 10.1177/00375497211068820 is OK
- 10.1007/978-3-030-61255-9_30 is OK
- 10.1145/375735.376120 is OK
- 10.1007/0-387-26350-0_7 is OK
- 10.1137/141000671 is OK

🟡 SKIP DOIs

- No DOI given, and none found for title: Artificial intelligence a modern approach
- No DOI given, and none found for title: Netlogo: A simple environment for modeling complex...
- No DOI given, and none found for title: JIAC V: A MAS framework for industrial application...
- No DOI given, and none found for title: agentframework (2.0.1) [last access 07-08-2024]

❌ MISSING DOIs

- None

❌ INVALID DOIs

- None
jbytecode commented 4 weeks ago

@editorialbot generate pdf

editorialbot commented 4 weeks ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

jbytecode commented 4 weeks ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

jbytecode commented 3 weeks ago

@jsagerOffis - While I am proofreading the whole stuff, you can follow the steps given in the above post. Thank you in advance.

jbytecode commented 3 weeks ago

@jsagerOffis - The default branch of the paper is set to joss_paper in this thread. However, the software repository doesn't have a branch of this name. Could you please clarify this issue? I need to fork the repo for some minor changes and I don't want to edit wrong files in wrong places. Thank you in advance.

jsagerOffis commented 3 weeks ago

@jbytecode Yeah sorry. It seems like I accidentally deleted the branch while merging the changes over to main for the release 🙈. I just recreated it from main.

jsagerOffis commented 3 weeks ago

@jbytecode

jbytecode commented 3 weeks ago

@editorialbot set v0.4.0 as version