diff --git a/SCRATCHPAD.md b/SCRATCHPAD.md index 28da596..1315a27 100644 --- a/SCRATCHPAD.md +++ b/SCRATCHPAD.md @@ -14,6 +14,7 @@ - `FileProperties` now uses one cached `ffprobe -show_format -show_streams -of json` call per source file, and the combined payload was confirmed against the Dragonball asset to satisfy both previous probe call sites fully. - Database startup now bootstraps schema only when required tables are actually missing, while version enforcement still runs on ordinary DB-backed context creation. - Helper filename and rich-text utilities now use compiled raw regexes plus translate-based filename filtering, with unit coverage for TMDB suffix rewriting and Rich color stripping. +- Process resource limiting now has explicit disabled/default states in the CLI and requirements, and combined CPU-plus-niceness wrapping now executes as `cpulimit -- nice -n ... ` instead of a less explicit prefix chain. - FFX logger setup now reuses named handlers, and fallback logger access no longer mutates handlers in ordinary constructors and helpers. - The process wrapper now uses `subprocess.run(...)` with centralized command formatting plus stable timeout and missing-command error mapping. - Active ORM controllers now use single-query accessors instead of paired `count()` plus `first()` lookups. @@ -87,15 +88,6 @@ - Faster contributor onboarding. - Easier CI adoption later. -7. Process resource limiting semantics could be clearer -- [`src/ffx/process.py`](/home/osgw/.local/src/codex/ffx/src/ffx/process.py) prepends `nice` and `cpulimit` directly when values are set. -- Optimization: - - Validate and document effective behavior for combined `nice` + `cpulimit`. - - Consider explicit no-limit vs configured-limit states in the CLI and requirements. -- Expected value: - - Fewer surprises in production-like runs. - - Easier support for user-reported performance behavior. - ## Open - Should optimization work focus first on operator-perceived latency, internal maintainability, or correctness-risk cleanup that also has performance upside? diff --git a/requirements/architecture.md b/requirements/architecture.md index 4e7f8e9..e5c86b4 100644 --- a/requirements/architecture.md +++ b/requirements/architecture.md @@ -42,7 +42,7 @@ - SQLite via SQLAlchemy ORM, with schema rooted in shows, patterns, tracks, media tags, track tags, shifted seasons, and generic properties. - A configuration JSON file supplies optional path, metadata-filtering, and filename-template settings. - Integration adapters: - - Process execution wrapper for `ffmpeg`, `ffprobe`, `nice`, and `cpulimit`. + - Process execution wrapper for `ffmpeg`, `ffprobe`, `nice`, and `cpulimit`, with explicit disabled states for niceness and CPU limiting and a combined `cpulimit -- nice -n ... ` execution shape when both limits are configured. - HTTP adapter for TMDB via `requests`. ## Data And Interface Notes diff --git a/requirements/project.md b/requirements/project.md index 2e8130c..2c38517 100644 --- a/requirements/project.md +++ b/requirements/project.md @@ -62,6 +62,10 @@ - The system shall support optional TMDB lookups to resolve show names, years, and episode titles when a show ID, season, and episode are available. - The system shall generate output filenames from show metadata, season and episode indices, and episode names using the configured filename template. - The system shall allow CLI overrides for stream languages, stream titles, default and forced tracks, stream order, TMDB show and episode data, output directory, label prefix, and processing resource limits. +- Processing resource limit rules: + - `--nice` shall accept niceness values from `-20` through `19`; omitting the option shall disable niceness adjustment. + - `--cpu` shall accept CPU limit values from `1` through `99`; omitting the option shall disable CPU limiting. + - When both limits are configured, the process wrapper shall execute the target command through `cpulimit` around a `nice -n ...` invocation so both limits apply to the launched media command. - The system shall support extracting streams into separate files via `unmux` and reporting suggested crop parameters via `cropdetect`. - The system shall handle invalid input and system failures gracefully by logging warnings or raising `click` errors for missing files, invalid media, missing TMDB credentials, incompatible database versions, and ambiguous track dispositions when prompting is disabled. diff --git a/src/ffx/cli.py b/src/ffx/cli.py index 189c198..4854297 100755 --- a/src/ffx/cli.py +++ b/src/ffx/cli.py @@ -32,6 +32,24 @@ if TYPE_CHECKING: LIGHTWEIGHT_COMMANDS = {None, 'version', 'help', 'configure_workstation', 'upgrade'} +def normalizeNicenessOption(ctx, param, value): + from ffx.process import normalizeNiceness + + try: + return normalizeNiceness(value) + except ValueError as ex: + raise click.BadParameter(str(ex)) from ex + + +def normalizeCpuOption(ctx, param, value): + from ffx.process import normalizeCpuPercent + + try: + return normalizeCpuPercent(value) + except ValueError as ex: + raise click.BadParameter(str(ex)) from ex + + @click.group() @click.pass_context @@ -194,6 +212,7 @@ def upgrade(ctx, branch): commandSequences += [ ['git', 'pull'], + [bundlePipPath, 'install', '--upgrade', 'pip', 'setuptools', 'wheel'], [bundlePipPath, 'install', '--editable', '.'], ] @@ -257,8 +276,22 @@ def getUnmuxSequence(trackDescriptor: TrackDescriptor, sourcePath, targetPrefix, @click.option('-l', '--label', type=str, default='', help='Label to be used as filename prefix') @click.option("-o", "--output-directory", type=str, default='') @click.option("-s", "--subtitles-only", is_flag=True, default=False) -@click.option('--nice', type=int, default=99, help='Niceness of started processes') -@click.option('--cpu', type=int, default=0, help='Limit CPU for started processes to percent') +@click.option( + '--nice', + type=int, + default=None, + callback=normalizeNicenessOption, + show_default='disabled', + help='Adjust niceness of started processes (-20..19). Omit to disable; 99 also disables.', +) +@click.option( + '--cpu', + type=int, + default=None, + callback=normalizeCpuOption, + show_default='disabled', + help='Limit CPU percent of started processes (1..99). Omit to disable; 0 also disables.', +) def unmux(ctx, paths, label, @@ -334,8 +367,22 @@ def unmux(ctx, @click.pass_context @click.argument('paths', nargs=-1) -@click.option('--nice', type=int, default=99, help='Niceness of started processes') -@click.option('--cpu', type=int, default=0, help='Limit CPU for started processes to percent') +@click.option( + '--nice', + type=int, + default=None, + callback=normalizeNicenessOption, + show_default='disabled', + help='Adjust niceness of started processes (-20..19). Omit to disable; 99 also disables.', +) +@click.option( + '--cpu', + type=int, + default=None, + callback=normalizeCpuOption, + show_default='disabled', + help='Limit CPU percent of started processes (1..99). Omit to disable; 0 also disables.', +) def cropdetect(ctx, paths, nice, @@ -479,8 +526,22 @@ def checkUniqueDispositions(context, mediaDescriptor: MediaDescriptor): @click.option("--no-signature", is_flag=True, default=False) @click.option("--keep-mkvmerge-metadata", is_flag=True, default=False) -@click.option('--nice', type=int, default=99, help='Niceness of started processes') -@click.option('--cpu', type=int, default=0, help='Limit CPU for started processes to percent') +@click.option( + '--nice', + type=int, + default=None, + callback=normalizeNicenessOption, + show_default='disabled', + help='Adjust niceness of started processes (-20..19). Omit to disable; 99 also disables.', +) +@click.option( + '--cpu', + type=int, + default=None, + callback=normalizeCpuOption, + show_default='disabled', + help='Limit CPU percent of started processes (1..99). Omit to disable; 0 also disables.', +) @click.option('--rename-only', is_flag=True, default=False, help='Only renaming, no recoding') diff --git a/src/ffx/process.py b/src/ffx/process.py index 7db5492..c186f9c 100644 --- a/src/ffx/process.py +++ b/src/ffx/process.py @@ -6,29 +6,72 @@ from .logging_utils import get_ffx_logger COMMAND_TIMED_OUT_RETURN_CODE = 124 COMMAND_NOT_FOUND_RETURN_CODE = 127 +MIN_NICENESS = -20 +MAX_NICENESS = 19 +DISABLED_NICENESS_SENTINEL = 99 +MIN_CPU_PERCENT = 1 +MAX_CPU_PERCENT = 99 +DISABLED_CPU_PERCENT_SENTINEL = 0 def formatCommandSequence(commandSequence: Iterable[str]) -> str: return shlex.join([str(token) for token in commandSequence]) +def normalizeNiceness(niceness) -> int | None: + if niceness is None: + return None + + niceness = int(niceness) + if niceness == DISABLED_NICENESS_SENTINEL: + return None + + if niceness < MIN_NICENESS or niceness > MAX_NICENESS: + raise ValueError( + f"Niceness must be between {MIN_NICENESS} and {MAX_NICENESS}, " + + f"or {DISABLED_NICENESS_SENTINEL} to disable." + ) + + return niceness + + +def normalizeCpuPercent(cpuPercent) -> int | None: + if cpuPercent is None: + return None + + cpuPercent = int(cpuPercent) + if cpuPercent == DISABLED_CPU_PERCENT_SENTINEL: + return None + + if cpuPercent < MIN_CPU_PERCENT or cpuPercent > MAX_CPU_PERCENT: + raise ValueError( + f"CPU limit must be between {MIN_CPU_PERCENT} and {MAX_CPU_PERCENT}, " + + f"or {DISABLED_CPU_PERCENT_SENTINEL} to disable." + ) + + return cpuPercent + + def getWrappedCommandSequence(commandSequence: List[str], context: dict = None) -> List[str]: """ - niceness -20 bis +19 - cpu_percent: 1 bis 99 + niceness: -20 to 19, disabled when unset + cpu_percent: 1 to 99, disabled when unset + + When both limits are configured, cpulimit wraps a nice-adjusted command: + cpulimit -l -- nice -n """ - niceSequence = [] + resourceLimits = (context or {}).get('resource_limits', {}) + niceness = normalizeNiceness(resourceLimits.get('niceness')) + cpu_percent = normalizeCpuPercent(resourceLimits.get('cpu_percent')) + wrappedCommandSequence = [str(token) for token in commandSequence] - niceness = int((context or {}).get('resource_limits', {}).get('niceness', 99)) - cpu_percent = int((context or {}).get('resource_limits', {}).get('cpu_percent', 0)) + if niceness is not None: + wrappedCommandSequence = ['nice', '-n', str(niceness)] + wrappedCommandSequence + if cpu_percent is not None: + wrappedCommandSequence = ['cpulimit', '-l', str(cpu_percent), '--'] + wrappedCommandSequence - if niceness >= -20 and niceness <= 19: - niceSequence += ['nice', '-n', str(niceness)] - if cpu_percent >= 1: - niceSequence += ['cpulimit', '-l', str(cpu_percent), '--'] - - return niceSequence + [str(token) for token in commandSequence] + return wrappedCommandSequence def getProcessTimeoutSeconds(context: dict = None, timeoutSeconds: float = None): diff --git a/tests/unit/test_cli_upgrade.py b/tests/unit/test_cli_upgrade.py index 90211ba..d392f27 100644 --- a/tests/unit/test_cli_upgrade.py +++ b/tests/unit/test_cli_upgrade.py @@ -87,6 +87,7 @@ class UpgradeCommandTests(unittest.TestCase): ['git', 'reset', '--hard', 'HEAD'], ['git', 'checkout', 'main'], ['git', 'pull'], + [pip_path, 'install', '--upgrade', 'pip', 'setuptools', 'wheel'], [pip_path, 'install', '--editable', '.'], ], [call[0] for call in subprocess_calls], diff --git a/tests/unit/test_process.py b/tests/unit/test_process.py index a379444..62ea007 100644 --- a/tests/unit/test_process.py +++ b/tests/unit/test_process.py @@ -15,6 +15,9 @@ from ffx.process import ( # noqa: E402 COMMAND_NOT_FOUND_RETURN_CODE, COMMAND_TIMED_OUT_RETURN_CODE, executeProcess, + getWrappedCommandSequence, + normalizeCpuPercent, + normalizeNiceness, ) @@ -47,6 +50,39 @@ class ProcessTests(unittest.TestCase): self.assertIn("Command timed out", err) self.assertIn(sys.executable, err) + def test_get_wrapped_command_sequence_leaves_command_unwrapped_when_limits_disabled(self): + wrapped = getWrappedCommandSequence( + ["ffmpeg", "-i", "input.mkv"], + context={"resource_limits": {"niceness": None, "cpu_percent": None}}, + ) + + self.assertEqual(["ffmpeg", "-i", "input.mkv"], wrapped) + + def test_get_wrapped_command_sequence_wraps_nice_when_configured(self): + wrapped = getWrappedCommandSequence( + ["ffmpeg", "-i", "input.mkv"], + context={"resource_limits": {"niceness": 5, "cpu_percent": None}}, + ) + + self.assertEqual(["nice", "-n", "5", "ffmpeg", "-i", "input.mkv"], wrapped) + + def test_get_wrapped_command_sequence_wraps_cpulimit_around_nice_when_both_configured(self): + wrapped = getWrappedCommandSequence( + ["ffmpeg", "-i", "input.mkv"], + context={"resource_limits": {"niceness": 5, "cpu_percent": 42}}, + ) + + self.assertEqual( + ["cpulimit", "-l", "42", "--", "nice", "-n", "5", "ffmpeg", "-i", "input.mkv"], + wrapped, + ) + + def test_normalize_niceness_accepts_disabled_sentinel(self): + self.assertIsNone(normalizeNiceness(99)) + + def test_normalize_cpu_percent_accepts_disabled_sentinel(self): + self.assertIsNone(normalizeCpuPercent(0)) + if __name__ == "__main__": unittest.main()