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: Let mesa runserver detect server.py as fallback #2015

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

rht
Copy link
Contributor

@rht rht commented Jan 28, 2024

This is part of the ongoing effort to obsolete run.py, to simplify the current model template. run.py always contains only 2 lines and hasn't been used for any customization, and so should be merged back into server.py. For discussion, see #1269.

@rht rht marked this pull request as ready for review January 28, 2024 11:23
@rht
Copy link
Contributor Author

rht commented Jan 28, 2024

Tested manually:

  • In the presence of both run.py and server.py
  • In the presence of server.py
  • In the presence of neither run.py nor server.py
    All the 3 cases works as expected.

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔵 -0.2% [-0.5%, +0.1%] 🔵 -0.2% [-0.4%, -0.1%]
Schelling large 🔵 -0.6% [-1.2%, +0.0%] 🔵 -0.4% [-1.6%, +0.7%]
WolfSheep small 🔵 +0.2% [-0.3%, +0.6%] 🔵 +0.4% [+0.3%, +0.5%]
WolfSheep large 🔵 +0.3% [-0.1%, +0.8%] 🔵 +0.4% [-0.3%, +1.2%]
BoidFlockers small 🔵 +1.9% [+1.3%, +2.5%] 🔵 -0.0% [-0.6%, +0.6%]
BoidFlockers large 🔵 +2.1% [+1.7%, +2.5%] 🔵 +0.4% [-0.0%, +0.9%]

Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
Schelling small 🔴 +3.7% [+3.1%, +4.3%] 🔵 +0.6% [+0.5%, +0.8%]
Schelling large 🔵 +6.6% [+2.3%, +11.2%] 🔵 +3.0% [-3.6%, +10.5%]
WolfSheep small 🔵 +0.0% [-0.5%, +0.6%] 🔵 -0.3% [-0.4%, -0.2%]
WolfSheep large 🔵 +2.0% [+1.3%, +2.9%] 🔵 +3.5% [+0.0%, +6.6%]
BoidFlockers small 🔵 +2.8% [+1.9%, +3.6%] 🔵 -1.0% [-1.6%, -0.4%]
BoidFlockers large 🔵 +2.6% [+2.0%, +3.3%] 🔵 -0.6% [-1.3%, +0.0%]

@rht
Copy link
Contributor Author

rht commented Jan 28, 2024

@EwoutH do you know why did the performance comment happen twice?

@EwoutH
Copy link
Member

EwoutH commented Jan 28, 2024

Once for opening the PR, once for marking it "Ready for review"

@tpike3 tpike3 added the feature Release notes label label Jan 29, 2024
@rht rht requested a review from jackiekazil January 31, 2024 21:14
@jackiekazil
Copy link
Member

I will take a look this evening or tomorrow. This is exciting.

@tpike3 tpike3 merged commit f1fcdc6 into projectmesa:main Feb 4, 2024
15 of 16 checks passed
@EwoutH EwoutH added enhancement Release notes label and removed feature Release notes label labels Feb 4, 2024
@EwoutH
Copy link
Member

EwoutH commented Feb 4, 2024

Can someone write a 3 sentence summary of this PR for the 2.2.5 release notes?

(I would find it really useful if each PR message is readable in isolation. Just from the PR message I would like to understand what a PR does/solves/enhances/adds without having to dig into code or other issues)

@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

This is a new feature, even though @tpike3 said that further older models used server.py instead of run.py, but the feature wasn't present in 2.2.4 nor 2.1.x and 2.0.x. And so can't be included in a patch release.

@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

But I suppose it is fine if the feature has once existed in the past? @EwoutH updated PR description. Up to you if you want to include in 2.2.5 or not.

@rht rht deleted the runserver branch February 4, 2024 21:41
@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

Wait, I just realized of a bug in this PR

In the presence of both run.py and server.py

The code says that both run.py and server.py are executed in sequence instead. Not sure why I didn't see it. Let me double check.

@rht
Copy link
Contributor Author

rht commented Feb 4, 2024

I see why the code passed my manual tests: it is because the first run call blocks the execution of the next line indefinitely, and the user always presses ctrl-c to exit before the next line gets executed. All is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Release notes label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants