Skip to content

Fix broken backends: PBS runs a nonexistent file, local auto-parallelization silently no-ops #120

Description

@jeremymanning

Part of #108 · Phase 3

Three backend defects that make advertised functionality fail. Grouped because each is small and they share test scaffolding.

1. PBS is broken — it runs a file that is never created

clustrix/utils.py:1226 — the generated PBS job script ends with:

python execute_function.py

execute_function.py is never written anywhere in the package. It is the only occurrence of that filename in clustrix/. Compare SSH (utils.py:1330-1452) and SGE (utils.py:1233-1330), which both generate a real inline runner.

Additionally, submit_pbs_job (executor_schedulers.py:157-198) never calls setup_remote_environment, yet the script it submits runs source venv/bin/activate (utils.py:1225) against a venv nothing created.

  • PBS generates a real inline runner that writes result.pkl, matching the SSH/SGE pattern
  • submit_pbs_job performs environment setup like its siblings
  • A test submits a real PBS job and asserts on the returned value

2. Local auto-parallelization is broken

clustrix/decorator.py:778 injects _parallel_{variable} into the kwargs passed to the user's function, then local_executor.py:128 calls self._executor.submit(func, *args, **kwargs). User functions do not accept a _parallel_* keyword, so this raises TypeError — which is then swallowed at decorator.py:708-716 and silently "falls back to sequential".

So the headline auto-parallelization feature silently does nothing, and the user is never told.

Related: decorator.py:375 _create_work_chunks is marked "a simplified implementation", and decorator.py:411 _combine_results says "For now, just return the list".

  • Chunk data is passed via a mechanism the user function can actually receive
  • The TypeError swallow is removed; a real failure surfaces as a failure
  • A test asserts that a parallelized loop actually ran in parallel and produced the correct combined result
  • _combine_results implements real combination, or the feature is documented as returning a list

3. cluster_type: "local" is accepted but unsupported

ClusterExecutor has no local branch (executor_core.py:101), so a config that sets cluster_type="local" raises ValueError: Unsupported cluster type: local at execution time. This accounts for 7 of the 127 current test failures.

  • Either implement the local branch, or reject it at config-validation time with a message naming the supported types — accepted-then-crashed is the worst of both

Also in scope (small, same area)

  • executor_schedulers.py:243 — SGE job-ID parsing uses fragile positional string splitting of qsub output; make it robust and test it against real qsub output shapes
  • utils.py:81 — range_obj = eval(range_part), carrying the author's own comment # Dangerous in practice, needs safer evaluation. Replace with ast.literal_eval or a real parser

Activity

  1. added 4 commits that reference this issue on Aug 17, 2026
  2. jeremymanning commented on Aug 19, 2026

    @jeremymanning
    MemberAuthor

    Resolved — three fixed, two obsolete

    1. PBS ran python execute_function.py, a file nothing creates — obsolete.

    $ grep -rn "execute_function.py" clustrix/
    (no matches)
    

    The PBS backend was removed in PR #149 (#140). It was also the one scheduler that never set up its remote environment; that was corrected during the v0.2.0 sweep, but the correction itself was never run against a real PBS queue, which is why the backend went.

    2. Local auto-parallelization silently no-opped — FIXED.
    It injected a _parallel_<var> keyword the callee could not accept, then swallowed the resulting TypeError and ran sequentially. Now decorator.py:315 _accepts_chunk_kwargs checks the signature first and declines with a log line (:354-370), and decorator.py:475-480 re-raises TypeError instead of swallowing it. Covered by tests/unit/test_local_auto_parallel.py.

    3. cluster_type="local" raised ValueError: Unsupported cluster type — FIXED.
    executor_core.py:65-67 dispatches to LocalJobManager.

    4. SGE status parsing — obsolete. Backend removed (#141).

    5. eval(range_part) on text sliced out of user source — FIXED.
    Replaced by SafeRangeEvaluator (loop_analysis.py:238), a literal-only reader that cannot execute anything.

    $ grep -rn "[^.a-z_]eval(" clustrix/
    (only two occurrences, both in comments describing the old behaviour)
    

    The same change removed the range(10) fallback that silently gave callers a tenth of their work.

    On _combine_results: it still returns the ordered list of per-chunk values. decorator.py:294 documents that as the deliberate contract, and docs/source/limitations.rst warns that a parallel run can therefore return a different shape from a sequential one. That satisfies this issue's "fix it or document it" branch by the second route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P1-highRequired for production readinessbug

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions