Skip to content

update resource monitoring for multilib#1150

Merged
PhilipDeegan merged 1 commit intoPHAREHUB:masterfrom
PhilipDeegan:mon
Feb 11, 2026
Merged

update resource monitoring for multilib#1150
PhilipDeegan merged 1 commit intoPHAREHUB:masterfrom
PhilipDeegan:mon

Conversation

@PhilipDeegan
Copy link
Member

@PhilipDeegan PhilipDeegan commented Feb 10, 2026

fix some oversights

@coderabbitai
Copy link

coderabbitai bot commented Feb 10, 2026

📝 Walkthrough

Walkthrough

Refactored monitoring functions to remove cpplib parameter dependency by using direct pyphare.cpp module import and mpi_rank() for rank information. Updated all call sites in the simulator module to match new signatures. Added barrier synchronization before monitoring shutdown.

Changes

Cohort / File(s) Summary
Monitoring Module Refactoring
pyphare/pyphare/simulator/monitoring.py
Removed cpplib parameter from four function signatures (monitoring_yaml_file, setup_monitoring, monitoring_shutdown, timing_setup). Added import pyphare.cpp as cpp and replaced cpplib-based access with cpp.mpi_rank(). Added mpi_barrier() call in shutdown when stats manager exists; added error logging for timing directory creation failures.
Simulator Call Site Updates
pyphare/pyphare/simulator/simulator.py
Updated make_cpp_simulator() and Simulator.run() to invoke monitoring functions without cpplib parameter. Modified setup_monitoring() call to derive interval from monitoring parameter (default 100 if non-integer).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • nicolasaunai
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description 'fix some oversights' is vague and generic, providing no meaningful information about what specific issues are being addressed in the changeset. Provide a more descriptive explanation of which oversights are being fixed and why these monitoring function signature changes are necessary.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title relates to the changeset by mentioning resource monitoring updates, and the changes do refactor monitoring functions for multilib support by removing cpplib parameters and using cpp module directly.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pyphare/pyphare/simulator/monitoring.py (1)

64-69: ⚠️ Potential issue | 🟡 Minor

FileNotFoundError is unlikely from Path.mkdir(parents=True, exist_ok=True).

With parents=True and exist_ok=True, mkdir won't raise FileNotFoundError. The realistic failure modes here are PermissionError or OSError (e.g., read-only filesystem). Consider catching a broader OSError instead.

Proposed fix
     try:
         Path(".phare/timings").mkdir(parents=True, exist_ok=True)
-    except FileNotFoundError:
+    except OSError:
         logger.error(f"Couldn't find timing dir from {os.getcwd() }")
🤖 Fix all issues with AI agents
In `@pyphare/pyphare/simulator/simulator.py`:
- Around line 202-203: The current check uses isinstance(monitoring, int) which
treats True as an int (1); change the logic so boolean True/False are not
treated as numeric intervals: explicitly detect and skip booleans (e.g., if
isinstance(monitoring, int) and not isinstance(monitoring, bool) or
alternatively check isinstance(monitoring, bool) first), then set interval =
monitoring only for real ints, otherwise default to 100; update the line that
assigns interval and keep the subsequent mon.setup_monitoring(interval) call
unchanged so mon.setup_monitoring gets the correct default when monitoring is
True.

@PHAREHUB PHAREHUB deleted a comment from coderabbitai bot Feb 11, 2026
@PhilipDeegan PhilipDeegan merged commit 8e61fa3 into PHAREHUB:master Feb 11, 2026
10 checks passed
@PhilipDeegan PhilipDeegan deleted the mon branch February 11, 2026 14:16
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.

2 participants