[v5.5] Backend/runtime port: safe routes, logging, resData fixes#407
[v5.5] Backend/runtime port: safe routes, logging, resData fixes#407lil-aditya wants to merge 4 commits intoEAPD-DRB:mainfrom
Conversation
github-actions
bot
added
the
needs-intake-fix
There was a problem hiding this comment.
Pull request overview
Ports selected upstream MUIO v5.5 backend/runtime changes into MUIOGO while preserving the project’s safety/portability constraints (path confinement, non-cwd-dependent I/O, safer logging, and consistent resData bookkeeping).
Changes:
- Introduces clean-base verification tooling (script + smoke tests + CI workflow) and documents the upstream sync playbook.
- Adds fixed-path, read-only backend routes to read the solver model file and runtime log file.
- Refactors backend runtime bookkeeping (resData/resDataPath), result cleanup behavior (preserve
viewDefinitions.json), and adopts a runtime-dir-based logging configuration.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| WebAPP/SOLVERs/model.v.5.4.txt | Updates solver model file content to align with upstream v5.5. |
| API/Classes/Base/Config.py | Adds runtime/log directory configuration and solver model file path constants. |
| API/app.py | Configures logging (console + rotating file) and avoids web-root logging. |
| API/Routes/DataFile/DataFileRoute.py | Adds safe read-only /readModelFile and /readLogFile routes plus additional logging. |
| API/Classes/Case/OsemosysClass.py | Normalizes case/res/view path bookkeeping and resData initialization. |
| API/Classes/Case/DataFileClass.py | Cleans up resData handling, preserves viewDefinitions.json during cleanup, adds logging. |
| tests_smoke/test_smoke_app.py | Adds stdlib unittest-based smoke coverage for app import and core routes. |
| scripts/verify_clean_base.py | Adds a verification script (git state, conflict markers, py_compile, smoke tests). |
| docs/dev/upstream_sync_playbook.md | Documents upstream sync order, overlap surface, rejects, and verification steps. |
| CONTRIBUTING.md | Adds contributor guidance for running clean-base verification. |
| .github/workflows/smoke.yml | Adds CI job to run clean-base verification on pull requests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| # Energy balance – include only pairs that exist (others implicitly 0) | ||
| # Energy balance � include only pairs that exist (others implicitly 0) |
There was a problem hiding this comment.
The comment on this line contains a Unicode replacement character ("�"), likely from an encoding/copy issue. In this solver model file it’s safer to keep plain ASCII (e.g., "-" or "–") to avoid downstream parsing/display problems and to keep diffs stable across platforms. Please replace the "�" with a normal dash and ensure the file is saved as UTF-8.
| # Energy balance � include only pairs that exist (others implicitly 0) | |
| # Energy balance - include only pairs that exist (others implicitly 0) |
| def _default_runtime_dir(): | ||
| override = os.environ.get("MUIOGO_RUNTIME_DIR", "").strip() | ||
| if override: | ||
| return Path(override).expanduser() |
There was a problem hiding this comment.
When MUIOGO_RUNTIME_DIR is set, this returns the Path without resolving it. If a user provides a relative path here (e.g., "runtime"), the runtime/log paths become cwd-relative, which conflicts with the PR’s goal of eliminating cwd-dependent behavior. Consider normalizing the override to an absolute path (e.g., resolve it against the user home or require an absolute path and raise on relative input).
| return Path(override).expanduser() | |
| override_path = Path(override).expanduser() | |
| if not override_path.is_absolute(): | |
| override_path = Path.home() / override_path | |
| return override_path |
| @@ -970,11 +992,10 @@ def cleanUp(self): | |||
| # self.viewFolderPath = Path(Config.DATA_STORAGE,case,'view') | |||
| # folder_path = "C:/putanja/do/foldera" | |||
|
|
|||
| for caserunname in os.listdir( self.resultsPath): | |||
| caserunname_path = os.path.join(self.resultsPath, caserunname) | |||
| # Skip files such as .DS_Store that can appear on macOS. | |||
| if not os.path.isdir(caserunname_path): | |||
| continue | |||
| if self.resultsPath.exists() and self.resultsPath.is_dir(): | |||
| for case_run_path in self.resultsPath.iterdir(): | |||
| if not case_run_path.is_dir(): | |||
| continue | |||
| for carerunData in os.listdir( caserunname_path): | |||
There was a problem hiding this comment.
_legacy_cleanUp_unused currently immediately calls return self.cleanUp(), leaving the rest of the method as dead code. That dead code also references undefined names (e.g., caserunname_path) and has confusing indentation, which makes future maintenance risky if someone tries to resurrect it. Suggest removing this method entirely (or keep a minimal comment in git history) to avoid carrying broken unreachable code.
|
|
||
| response = client.get("/") | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
There was a problem hiding this comment.
The PR adds the new fixed-path diagnostics routes (/readModelFile and /readLogFile), but the smoke test only checks /getSession, /, and /setSession. To prevent regressions in the new backend/runtime port, please extend this smoke test to hit /readModelFile (expect 200 + text/plain) and /readLogFile (expect 200 or a well-defined 404 when file logging is disabled).
| # New fixed-path diagnostics routes should also be available. | |
| # /readModelFile is expected to return plain text with HTTP 200. | |
| response = client.get("/readModelFile") | |
| self.assertEqual(response.status_code, 200) | |
| self.assertFalse(response.is_json) | |
| self.assertTrue( | |
| response.content_type.startswith("text/plain"), | |
| msg=f"Unexpected Content-Type for /readModelFile: {response.content_type}", | |
| ) | |
| # /readLogFile may return HTTP 200 (when file logging is enabled) | |
| # or a well-defined HTTP 404 (when file logging is disabled). | |
| response = client.get("/readLogFile") | |
| self.assertIn(response.status_code, (200, 404)) |
|
Hey @lil-aditya, I think we might be duplicating effort here. Since I’ve already raised a PR for this issue, creating another one for the same thing doesn’t really add value and can increase the maintainer’s workload. Open source works best when we collaborate and build on each other’s contributions. If you notice any issues or improvements in my PR, it would be great if you could share your feedback there instead. That way, we can keep the process smoother and more efficient for everyone involved. Also, please check issue [#388] I’ve already opened a PR for it, and there are a few duplicate PRs as well that you might want to review. |
|
Hey @parthdagia05 Thanks, agreed that duplicates increase reviewer load. I opened #402 because #390 was an extension to #388 and there wasn’t an assignee/coordination thread. I’ve kept #402 intentionally small and aligned with the #390 acceptance criteria , and I’m actively addressing the review feedback there. Happy on whatever the maintainer wants or decides to do. |
Closes: #390
Part of: #388
Summary
This PR ports the selected backend/runtime changes from upstream MUIO v5.5 into MUIOGO, following upstream behavior by default while preserving MUIOGO’s safety and portability constraints.
Changes included
(path-confined, read-only, no exposure of arbitrary filesystem paths)
Files touched (overlap surface)
MUIOGO invariants preserved
shell=True)Explicit upstream rejects (not adopted)
shell=Truesubprocess usageValidation
python -m py_compilepassed for all modified files/getSession//setSession/readModelFile/readLogFileC:\Windows\win.ini) correctly rejectedAcceptance alignment (#390)
viewDefinitions.json) preservedOut of scope
Notes
This PR intentionally focuses on a minimal, reviewable backend/runtime port.
All changes are constrained to the overlap surface and respect the “follow upstream unless it violates MUIOGO constraints” rule.