refactor(sim): make lora mesh cli importable#67
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e82dfedcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if parsed_arguments.period_seconds is not None: | ||
| if not math.isfinite(parsed_arguments.period_seconds) or parsed_arguments.period_seconds <= 0: | ||
| parser.error("--period-seconds must be a positive finite number") | ||
| period = int(parsed_arguments.period_seconds * conf.ONE_SECOND_INTERVAL) |
There was a problem hiding this comment.
The new --period-seconds path validates only the float input and then truncates to milliseconds with int(...); values between 0 and 0.001 seconds become 0 here, which later propagates into node configs and causes expovariate(1.0 / period) to divide by zero in MeshNode.get_next_time during simulation startup. This makes a CLI invocation that passes validation fail at runtime, so the parser should reject values that round to < 1 tick (and similarly for simtime if zero-length runs are not intended).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah, well probably good to check for less than let's say 0.01 seconds here, because that is ridiculously low for LoRa anyway.
| # Keep the heavier simulation/GUI import out of module import. That makes | ||
| # CLI parsing unit-testable and lets CI/tools import loraMesh without | ||
| # starting Matplotlib/Tk plumbing as a side effect. | ||
| from lib.discrete_event_sim import DiscreteEventSim |
There was a problem hiding this comment.
run_simulation always imports DiscreteEventSim before checking conf.GUI_ENABLED, but lib/discrete_event_sim.py imports lib.gui at module load time; lib.gui hard-requires TkAgg and exits if Tk is unavailable. As a result, --no-gui still fails in headless/CI environments without tkinter, which contradicts the new headless CLI behavior introduced in this change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is indeed correct, DiscreteEventSim currently always requires lib.gui.
|
Thanks for this split, this is much easier to go through. I like the additional parameters, and also that you can run it without using Tk. Can you have a look at the two comments? They seem both valid to me. |
6e82dfe to
82ffb41
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82ffb41c7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| simtime = conf.SIMTIME | ||
| period = conf.PERIOD | ||
| gui_enabled = conf.GUI_ENABLED | ||
| plot_enabled = conf.PLOT |
There was a problem hiding this comment.
The new importable entrypoint now allows multiple main()/parse_params() calls in one process, but this code derives option defaults from the already-mutated conf object (SIMTIME, PERIOD, GUI_ENABLED, PLOT). After one run with overrides (for example --no-gui or --simtime-seconds), a later run without those flags silently inherits the previous values, producing incorrect experiment settings and headless behavior unexpectedly. This is a regression in the new reusable CLI flow because accepted runs are no longer isolated from each other.
Useful? React with 👍 / 👎.
82ffb41 to
e39a2ac
Compare
GUVWAF
left a comment
GUVWAF
left a comment
There was a problem hiding this comment.
Thanks! I tested this and it looks good to merge now!
2 participants
Summary
loraMesh.pyimportable by moving CLI startup intomain()and lazy-loading simulation/GUI plumbing--no-gui,--simtime-seconds, and--period-secondsfor headless smoke/CI runsValidation
.venv/bin/python -m unittest tests.test_lora_mesh_cli.venv/bin/python -m unittest/home/kom/.local/bin/ruff check loraMesh.py lib/config.py tests/test_lora_mesh_cli.py.venv/bin/python -m compileall -q ..venv/bin/python -u loraMesh.py 2 --no-gui --simtime-seconds 5 --period-seconds 0.5git diff --checkSplit from draft rollup #64.