Uh oh!
There was an error while loading. Please reload this page.
Test with SLURM - #726
Conversation
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA new GitHub Actions job Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Slurm as Slurm Setup Action
participant Conda as Conda Setup
participant Tests as Test Runner
CI->>Slurm: Setup Slurm environment (MPICH)
CI->>Conda: Install Python 3.13 and dependencies (env file)
CI->>Tests: Install package (no deps/build isolation)
CI->>Tests: Run Slurm executor test modules
Tests->>Slurm: Submit and manage jobs via Slurm
Slurm->>Tests: Return test results
Tests->>CI: Report pass/fail status
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
tests/test_slurmjobexecutor.py (2)
1-1: Remove unused import.The
osmodule is imported but never used in this file.-import os
10-13: Simplify conditional assignment.Use a more concise ternary operator for better readability.
-if shutil.which("srun") is not None:- skip_slurm_test = False-else:- skip_slurm_test = True+skip_slurm_test = shutil.which("srun") is Nonetests/test_slurmclusterexecutor.py (3)
9-12: Simplify conditional assignment.Use a more concise ternary operator for better readability.
-if shutil.which("srun") is not None:- skip_slurm_test = False-else:- skip_slurm_test = True+skip_slurm_test = shutil.which("srun") is None
25-28: Fix inconsistent skip message.The skip message mentions "h5py" but the skip condition doesn't check for h5py availability.
- "h5py or mpi4py or SLRUM are not installed, so the h5py, slurm and mpi4py tests are skipped.",+ "mpi4py or SLURM are not installed, so the slurm and mpi4py tests are skipped.",Also note the typo "SLRUM" should be "SLURM".
29-29: Consider renaming test class for clarity.The class name
TestCacheExecutorPysqadoesn't clearly indicate it's testingSlurmClusterExecutor.-class TestCacheExecutorPysqa(unittest.TestCase):+class TestSlurmClusterExecutor(unittest.TestCase):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pipeline.yml(2 hunks)tests/test_slurmclusterexecutor.py(1 hunks)tests/test_slurmjobexecutor.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_slurmclusterexecutor.py (4)
executorlib/executor/slurm.py (1)
SlurmClusterExecutor(23-205)executorlib/standalone/serialize.py (1)
cloudpickle_register(9-28)tests/test_slurmjobexecutor.py (1)
mpi_funct(20-25)executorlib/task_scheduler/file/hdf.py (1)
dump(11-28)
tests/test_slurmjobexecutor.py (2)
executorlib/executor/slurm.py (1)
SlurmJobExecutor(208-387)tests/test_slurmclusterexecutor.py (1)
mpi_funct(17-22)
🪛 Ruff (0.11.9)
tests/test_slurmclusterexecutor.py
9-12: Use ternary operator skip_slurm_test = False if shutil.which("srun") is not None else True instead of if-else-block
Replace if-else-block with skip_slurm_test = False if shutil.which("srun") is not None else True
(SIM108)
75-75: Undefined name dump
(F821)
tests/test_slurmjobexecutor.py
1-1: os imported but unused
Remove unused import: os
(F401)
10-13: Use ternary operator skip_slurm_test = False if shutil.which("srun") is not None else True instead of if-else-block
Replace if-else-block with skip_slurm_test = False if shutil.which("srun") is not None else True
(SIM108)
🔇 Additional comments (3)
.github/workflows/pipeline.yml (1)
267-330: LGTM on the Slurm test job structure.The new Slurm unittest jobs follow the established pattern and include appropriate MySQL service setup and environment configuration.
tests/test_slurmjobexecutor.py (1)
39-119: Excellent test coverage for SlurmJobExecutor.The test suite comprehensively covers various execution scenarios including serial, parallel, threaded execution, mapping, internal memory handling, and parameter validation.
tests/test_slurmclusterexecutor.py (1)
30-91: Well-structured tests for SlurmClusterExecutor.The tests effectively cover different executor configurations, cache directory management, and file handling scenarios.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertTrue(fs_1.done()) | ||
| self.assertTrue(fs_2.done()) | ||
| def test_slurm_executor_serial_no_depencies(self): |
There was a problem hiding this comment.
Fix typo in method name.
The method name has a typo: "depencies" should be "dependencies".
- def test_slurm_executor_serial_no_depencies(self):+ def test_slurm_executor_serial_no_dependencies(self):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deftest_slurm_executor_serial_no_depencies(self): | |
| deftest_slurm_executor_serial_no_dependencies(self): |
🤖 Prompt for AI Agents
In tests/test_slurmjobexecutor.py at line 52, the test method name contains a
typo: "depencies" should be corrected to "dependencies". Rename the method from
test_slurm_executor_serial_no_depencies to
test_slurm_executor_serial_no_dependencies to fix the spelling error.
Uh oh!
There was an error while loading. Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #726 +/- ##
=======================================
Coverage 96.91% 96.91% =======================================
Files 30 30 Lines 1363 1363 =======================================
Hits 1321 1321 Misses 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_slurmjobexecutor.py (1)
51-51: Fix typo in method name.The method name has a typo: "depencies" should be "dependencies".
- def test_slurm_executor_serial_no_depencies(self):+ def test_slurm_executor_serial_no_dependencies(self):
🧹 Nitpick comments (1)
tests/test_slurmclusterexecutor.py (1)
89-89: Remove extra space before closing parenthesis.- file_path = os.path.join("executorlib_cache", file_name )+ file_path = os.path.join("executorlib_cache", file_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pipeline.yml(2 hunks)tests/test_slurmclusterexecutor.py(1 hunks)tests/test_slurmjobexecutor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pipeline.yml
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_slurmclusterexecutor.py (4)
executorlib/executor/slurm.py (1)
SlurmClusterExecutor(23-205)executorlib/standalone/serialize.py (1)
cloudpickle_register(9-28)executorlib/task_scheduler/file/hdf.py (1)
dump(11-28)tests/test_slurmjobexecutor.py (1)
mpi_funct(20-25)
🪛 Ruff (0.11.9)
tests/test_slurmclusterexecutor.py
9-12: Use ternary operator skip_slurm_test = False if shutil.which("srun") is not None else True instead of if-else-block
Replace if-else-block with skip_slurm_test = False if shutil.which("srun") is not None else True
(SIM108)
tests/test_slurmjobexecutor.py
1-1: os imported but unused
Remove unused import: os
(F401)
10-13: Use ternary operator skip_slurm_test = False if shutil.which("srun") is not None else True instead of if-else-block
Replace if-else-block with skip_slurm_test = False if shutil.which("srun") is not None else True
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_slurm_mpich
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_win
- GitHub Check: notebooks_integration
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: notebooks
🔇 Additional comments (5)
tests/test_slurmclusterexecutor.py (3)
9-13: The skip flag logic is correct.The conditional check for
srunavailability is appropriate for skipping tests when SLURM is not installed.
23-33: SLURM submission template is properly configured.The template includes all necessary SLURM directives and placeholders for dynamic values.
48-106: Test methods comprehensively cover SlurmClusterExecutor functionality.The tests validate:
- Basic MPI execution with cache management
- Execution without working directory specification
- Cache reuse with existing files
All assertions and test patterns are appropriate.
tests/test_slurmjobexecutor.py (2)
16-34: Helper functions are well-designed for testing.The functions provide appropriate test cases for serial execution, MPI parallelism, and internal memory management.
40-105: Comprehensive test coverage for SlurmJobExecutor.The test methods effectively validate:
- Serial execution with and without dependencies
- Thread-level parallelism
- MPI parallel execution
- Batch job submission via map
- Internal memory initialization and persistence
All test patterns and assertions are appropriate.
Uh oh!
There was an error while loading. Please reload this page.
jan-janssen
commented
Jul 13, 2025
@liamhuber I added a first set of tests for the |
liamhuber
left a comment
There was a problem hiding this comment.
Nice, this will indeed be helpful for me to test pyiron_workflow's use of executorlib!
I have security nit thought, a GitHub script simplification to try, and a question, but none of it is blocking.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
based on Jan's work in pyiron/executorlib#726 Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
Summary by CodeRabbit