[Fix] Rename run_time_limit to run_time_max#953
Conversation
Primarily for consistency with pysqa.
📝 WalkthroughWalkthroughThe pull request renames the resource dictionary key and related parameters from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #953 +/- ## ======================================= Coverage 94.14% 94.14% ======================================= Files 39 39 Lines 2082 2082 ======================================= Hits 1960 1960 Misses 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/executorlib/standalone/validate.py (1)
26-26: Add a deprecation bridge forrun_time_limitto avoid abrupt API breakage.Right now, callers still sending
run_time_limitcan fail validation or propagate an unrecognized key downstream. Consider normalizing it torun_time_maxwith a deprecation warning for one transition window.♻️ Proposed compatibility shim
import warnings from typing import Optional @@ class ResourceDictValidation(BaseModel): @@ run_time_max: Optional[int] = None @@ def _get_accepted_keys(class_type) -> list[str]: @@ raise TypeError("Unsupported class type for validation") + + +def _normalize_runtime_key(resource_dict: dict) -> None: + if "run_time_limit" in resource_dict and "run_time_max" not in resource_dict: + warnings.warn( + "`run_time_limit` is deprecated; use `run_time_max`.", + stacklevel=2, + ) + resource_dict["run_time_max"] = resource_dict.pop("run_time_limit") @@ def validate_resource_dict(resource_dict: dict) -> None: + _normalize_runtime_key(resource_dict=resource_dict) _ = ResourceDictValidation(**resource_dict) @@ def validate_resource_dict_with_optional_keys(resource_dict: dict) -> None: + _normalize_runtime_key(resource_dict=resource_dict) accepted_keys = _get_accepted_keys(class_type=ResourceDictValidation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/executorlib/standalone/validate.py` at line 26, Add a short compatibility shim that accepts the legacy key run_time_limit and maps it to the new field run_time_max during validation: in validate.py add a normalization step (before the existing schema/dataclass validation) that checks for run_time_limit in the incoming params dict, if run_time_max is not already set copy run_time_limit -> run_time_max, emit a DeprecationWarning or a logged warning calling warnings.warn("run_time_limit is deprecated; use run_time_max", DeprecationWarning) and then remove run_time_limit from the dict so downstream code only sees run_time_max; place this logic near the existing validation entrypoint that currently defines run_time_max: Optional[int] = None so callers keep working during the transition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Nitpick comments: In `@src/executorlib/standalone/validate.py`: - Line 26: Add a short compatibility shim that accepts the legacy key run_time_limit and maps it to the new field run_time_max during validation: in validate.py add a normalization step (before the existing schema/dataclass validation) that checks for run_time_limit in the incoming params dict, if run_time_max is not already set copy run_time_limit -> run_time_max, emit a DeprecationWarning or a logged warning calling warnings.warn("run_time_limit is deprecated; use run_time_max", DeprecationWarning) and then remove run_time_limit from the dict so downstream code only sees run_time_max; place this logic near the existing validation entrypoint that currently defines run_time_max: Optional[int] = None so callers keep working during the transition. ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c521fc42-604c-4e10-a37d-1c307eaaf795
📒 Files selected for processing (13)
docs/trouble_shooting.mdsrc/executorlib/executor/flux.pysrc/executorlib/executor/single.pysrc/executorlib/executor/slurm.pysrc/executorlib/standalone/command.pysrc/executorlib/standalone/validate.pysrc/executorlib/task_scheduler/file/spawner_pysqa.pysrc/executorlib/task_scheduler/interactive/spawner_flux.pysrc/executorlib/task_scheduler/interactive/spawner_pysqa.pysrc/executorlib/task_scheduler/interactive/spawner_slurm.pytests/unit/executor/test_flux_job.pytests/unit/standalone/test_slurm_command.pytests/unit/standalone/test_validate.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@src/executorlib/executor/flux.py`: - Line 420: Add a deprecation shim that detects the old key "run_time_limit" in resource dicts before validation and maps it to the new "run_time_max" while emitting a deprecation warning: modify the code path used by FluxJobExecutor and FluxClusterExecutor to check incoming resource dicts (e.g., inside the place that calls validate_resource_dict_with_optional_keys) for "run_time_limit", log a clear deprecation warning, copy its value to "run_time_max" (if "run_time_max" not already present) and remove "run_time_limit" so the existing validate_resource_dict_with_optional_keys behavior continues to work without silently dropping the limit. ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecf7c9ad-8969-49d2-b9d6-b861fb5b3519
📒 Files selected for processing (3)
src/executorlib/executor/flux.pysrc/executorlib/executor/slurm.pysrc/executorlib/task_scheduler/interactive/spawner_pysqa.py
✅ Files skipped from review due to trivial changes (1)
- src/executorlib/executor/slurm.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/executorlib/task_scheduler/interactive/spawner_pysqa.py
| "openmpi_oversubscribe": openmpi_oversubscribe, | ||
| "slurm_cmd_args": [], | ||
| "run_time_limit": None, | ||
| "run_time_max": None, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check if there's any deprecation handling for run_time_limit elsewhere in the codebase rg -n 'run_time_limit' --type py # Check if there are migration notes or changelog entries fd -e md -e rst -e txt | xargs rg -l 'run_time_limit\|run_time_max' 2>/dev/null || echo "No migration docs found"Repository: pyiron/executorlib
Length of output: 85
🏁 Script executed:
# First, let's see the actual lines around 420 in flux.py head -430 src/executorlib/executor/flux.py | tail -30Repository: pyiron/executorlib
Length of output: 1849
🏁 Script executed:
# Check the validation functions referenced in the comment rg -A 10 'def validate_resource_dict' --type pyRepository: pyiron/executorlib
Length of output: 1986
🏁 Script executed:
# Check if there are any tests that demonstrate the breaking behavior fd -e py -path '*/test*' | xargs rg -l 'run_time' 2>/dev/nullRepository: pyiron/executorlib
Length of output: 833
🏁 Script executed:
# Look for any migration or deprecation related code rg -i 'deprecat|migrat|compat' --type py | head -20Repository: pyiron/executorlib
Length of output: 2848
🏁 Script executed:
# Check FluxJobExecutor's __init__ around lines 155-165 to see which validation it uses sed -n '140,170p' src/executorlib/executor/flux.pyRepository: pyiron/executorlib
Length of output: 3374
🏁 Script executed:
# Find ResourceDictValidation schema definition rg -B 3 'class ResourceDictValidation' --type py -A 15Repository: pyiron/executorlib
Length of output: 1351
🏁 Script executed:
# Check if tests exist for run_time_limit behavior with both executors cat tests/unit/executor/test_flux_job.py | head -200Repository: pyiron/executorlib
Length of output: 6240
Breaking change: users passing run_time_limit will have it silently ignored.
The rename from run_time_limit to run_time_max is a breaking change. Both FluxJobExecutor and FluxClusterExecutor use validate_resource_dict_with_optional_keys, which issues a warning for unrecognized keys but silently filters them out. Existing users passing run_time_limit will see a warning and the key will be ignored—meaning the time limit won't be applied.
Consider adding a deprecation shim that detects run_time_limit, issues a deprecation warning, and maps it to run_time_max for a smoother migration path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/executorlib/executor/flux.py` at line 420, Add a deprecation shim that detects the old key "run_time_limit" in resource dicts before validation and maps it to the new "run_time_max" while emitting a deprecation warning: modify the code path used by FluxJobExecutor and FluxClusterExecutor to check incoming resource dicts (e.g., inside the place that calls validate_resource_dict_with_optional_keys) for "run_time_limit", log a clear deprecation warning, copy its value to "run_time_max" (if "run_time_max" not already present) and remove "run_time_limit" so the existing validate_resource_dict_with_optional_keys behavior continues to work without silently dropping the limit. 
| It seems to work in the sense that using now However, when I don't pass it (or actually I left it as limit because I forgot to change it), then the executor just runs directly in the notebook without submitting, which is very strange. |
This should not be the case, are you sure it is not just reloading the result from the cache folder? |
I thought I avoided that by renaming the cache folder between runs, but I'll try to set up a clean reproducer. It may well be I mixing something up, since I was trying a few thing on the go. |
2 participants
Primarily for consistency with pysqa.
Summary by CodeRabbit