Skip to content

Run PRK and PDIRK stages serially inside another threaded region - #4940

Open
singhharsh1708 wants to merge 5 commits into
SciML:masterfrom
singhharsh1708:core-threaded-nested
Open

singhharsh1708 wants to merge 5 commits into
SciML:masterfrom
singhharsh1708:core-threaded-nested

Conversation

@singhharsh1708

@singhharsh1708 singhharsh1708 commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

KuttaPRK2p5 and PDIRK44, which default to threading = true, now run their stages serially when the solve is already inside a threaded region, instead of throwing "@threads :static cannot be used concurrently or nested". OrdinaryDiffEqCore's _threaded_execute gets the same jl_in_threaded_region check that #4929 adds to the extrapolation methods' own helper.

Called from inside Threads.@threads (:static or :dynamic), both methods throw on master, in place and out of place; here they run and give the same u as the serial solve. The new PRK testset fails on master with 1 and with 4 threads and passes here. Core QA, PRK Core and PDIRK Core pass. PDIRK has no [sources], so its tests use the registered Core and are not a test of this change; I checked it with a local environment instead.

OrdinaryDiffEqCore goes to 4.20.2, the same version as #4921, #4931 and #4932.

AI Disclosure: Claude assisted with this change.

@singhharsh1708

Copy link
Copy Markdown
Member Author

🤖 Automated comment from an AI agent running as @singhharsh1708, not written or reviewed by the account holder.

Independent review (Codex CLI 0.163.0, model gpt-6.1-sol, reasoning effort high): CHANGES, risk medium-low (ML8). Reviewed head: 09cfafa8548872f2a59e76007bbfe17a4aeee2e1.

Full review

VERDICT: CHANGES
PR: #4940
Round: 1
Head: 09cfafa
Base: 45ff46a
Slot: 1
Reviewer: Codex CLI 0.163.0 / gpt-6.1-sol
Session: 01a19787-ffa7-7413-b9bf-f4f65b834d22
Reviewed: 2026-11-01T14:42:19Z
Summary: The PR serializes internally threaded work when another threaded region is active, fixing the reproduced PRK exception. Targeted PRK and PDIRK checks pass, but the regression is disconnected from CI and the implementation introduces a non-public runtime dependency.

Risk

medium-low (ML8: changes a shared threading helper; targeted runs establish preserved results and reproduce the original exception.)

Risk assessment

  • Risk: medium-low (ML8: shared threading behavior changes, with successful manual regression and equivalence checks.)
  • Blast radius: Bool/BaseThreads execution through Core’s @threaded, including PRK, PDIRK, optional threaded SDC, and AdaptiveRadau stepping/interpolation work.
  • Evidence: Changed PRK test passes all six assertions on head and errors on the copied base test. An additional PDIRK probe passes 17 checks. Seven CI jobs fail; six failure signatures match captured master failures.
  • Verdict: CHANGES
  • Round: 1
  • Blocking priorities: P2
  • Merge: needs changes (connect regression coverage to CI and remove the non-public runtime dependency)

Blocking findings

  1. [P2] Wire the new regression into the test target - confirmed by running. At lib/OrdinaryDiffEqPRK/test/prk_convergence.jl:21, the new test is added to a file that lib/OrdinaryDiffEqPRK/test/runtests.jl never includes; repository-wide searches found no other inclusion. Additionally, directly including it in the declared test environment exits before assertions with ArgumentError: Package ODEProblemLibrary not found in current path. Thus the green PRK CI jobs do not protect this threading fix. Include this file and declare its missing test dependency, or move the regression into an included test file. The regression itself works when its environment is repaired temporarily.

  2. [P2] Avoid the undocumented Julia runtime entry point - suspected from reading. lib/OrdinaryDiffEqCore/src/misc_utils.jl:127 directly calls jl_in_threaded_region. This is an undocumented runtime C symbol, absent from the installed public headers, rather than a supported public threading API. It violates the review’s public-API requirement; passing on Julia 1.13.1 does not establish a compatibility contract. Implement the behavior through documented nestable scheduling, such as Threads.@threads :dynamic, or obtain a public detection API upstream. Julia documents both its public API boundary and thread scheduling options.

Non-blocking findings

None.

What I ran

Commands below substitute $JRUN, $BASE_WORKTREE, and $PROBE for the supplied runner, supplied base checkout, and temporary PDIRK probe respectively. All Julia processes ran sequentially through the supplied wrapper: Julia 1.13.1, four Julia threads, one OpenBLAS thread, isolated temporary directories.

  • Git inspection, exit 0:

    git rev-parse HEAD
    git merge-base origin/master HEAD
    git diff origin/master...HEAD
    git log -1 --format=%B
    git diff --check
    git status --short

    Head and merge-base matched the requested SHAs. Head remained clean; no source files changed. The commit has no agent trailers.

  • Initial head test, exit 1:

    "$JRUN" --project=lib/OrdinaryDiffEqPRK -e 'using TestEnv; TestEnv.activate(); using Test, SafeTestsets; println("Julia ", VERSION); include("lib/OrdinaryDiffEqPRK/test/prk_convergence.jl")'

    Tail: ArgumentError: Package ODEProblemLibrary not found in current path.

  • Corrected temporary test environment, run identically from head and base:

    "$JRUN" --project=lib/OrdinaryDiffEqPRK -e 'using TestEnv; TestEnv.activate(); using Pkg; Pkg.add("ODEProblemLibrary"); using Test, SafeTestsets; println("Julia ", VERSION); import OrdinaryDiffEqCore; println("Core version ", pkgversion(OrdinaryDiffEqCore)); include("lib/OrdinaryDiffEqPRK/test/prk_convergence.jl")'

    Head: exit 0, Core 4.19.1:

    Explicit Solver Convergence Tests (out-of-place) | 2 passed
    Explicit Solver Convergence Tests (in-place)     | 2 passed
    Threaded stages inside another threaded loop    | 2 passed
    

    Before the base run, I copied the PR’s changed test into the supplied merge-base worktree and verified identical contents:

    cp lib/OrdinaryDiffEqPRK/test/prk_convergence.jl "$BASE_WORKTREE/lib/OrdinaryDiffEqPRK/test/prk_convergence.jl"
    cmp lib/OrdinaryDiffEqPRK/test/prk_convergence.jl "$BASE_WORKTREE/lib/OrdinaryDiffEqPRK/test/prk_convergence.jl"

    Both commands exited 0. Base test: exit 1, Core 4.19.0:

    Existing convergence assertions: 4 passed
    nested task error: `@threads :static` cannot be used concurrently or nested
    Threaded stages inside another threaded loop | 1 errored
    
  • Additional head probe, exit 0:

    "$JRUN" --project=lib/OrdinaryDiffEqPRK "$PROBE"

    The probe developed local PDIRK into a temporary TestEnv environment and asserted that local head Core loaded. It checked in-place/out-of-place problems, true/BaseThreads(), static/dynamic outer loops, exact serial equivalence, successful return codes, and convergence against exp(-1).

    PDIRK nested execution            | 16 passed
    convergence orders = [3.8663143999829117, 3.930599369225435]
    PDIRK nested analytic convergence | 1 passed
    
  • Inspected supplied CI results and failure excerpts for PR sublibraries, PR downstream tests, and PR main tests. Compared against master sublibraries, master downstream tests, and master main tests.

    Matching signatures cover SciMLSensitivity Core2/Core4, AD tape recording, both GPU deprecation-warning failures, and NonlinearSolve’s Newton line-search exception. ModelingToolkit InterfaceI’s reversed-equation assertion has no matching failure in the supplied master excerpts; its attribution remains unresolved.

What I did not verify

  • Julia 1.10 or other platforms locally.
  • Nested FIRK/AdaptiveRadau and SDC execution, or Polyester execution.
  • GPU, additional AD, MPI, package-wide, or heavy downstream suites.
  • The cause of the unmatched ModelingToolkit CI failure.

AUTHOR_GRADE: {"task":"bugfix","score":3,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The small threading fix passes targeted PRK and PDIRK checks, and the copied regression fails on base. Substantive repairs remain because the regression is disconnected from CI and the implementation relies on a non-public runtime symbol."}


🤖 Posted by an AI agent · harness: Claude Code · model: claude-opus-5-5 (coordinating gpt-6.1-sol reviewers)
Review: unreviewed
Session: 01a19787-ffa7-7413-b9bf-f4f65b834d22

@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

PR: #4940

Reviewed head: 1c61b6f
Raw SHA-256: 89e5908a1010cec3b39a21fa8c36c185afb425f7efe34b0106f176c6da383cbb
Source SHA-256: a41c90541d4e18d23b07c33f8de2f2999db04030d8358847f38c0500e40f3160
Source: standard-review-v1 (harness-log)
Reviewer session: 01a1d257-53e3-7382-ac1e-956a412a6a16
Slot: 1
Confidence: unknown
Own risk fixed before author rating/discussion: yes

Risk assessment

  • Risk: medium-low (ML8)
  • Verdict: CHANGES
  • Reviewed head: 1c61b6f

Review report

VERDICT: CHANGES
Round: 1
Head: 1c61b6f
Slot: 1
CI: CI is not green (regress:SciMLSensitivity.jl/Core4/1 / Downstream Tests - Core4)
Session: 01a1d257-53e3-7382-ac1e-956a412a6a16

Risk

medium-low (ML8: changes shared stage-threading execution for PRK, PDIRK and optional FIRK/SDC paths; adds a nested-threading regression test, but runtime verification is outstanding).

Blocking findings

  • Verdict: CHANGES
  • Round: 1
  • Blocking priorities: P2, P2
  • Deferred P2 issues: None supplied.

This is a needs-verification verdict. No numerical regression or other code correctness failure was confirmed by running.

  1. [P2] Required runtime evidence is unavailable — lib/OrdinaryDiffEqCore/src/misc_utils.jl:127 and lib/OrdinaryDiffEqPRK/test/runtests.jl:26. The affected suite and targeted probes could not start within the supplied resource limits. Consequently, I did not establish that the added test fails with the original helper and passes with this change, or that the fallback preserves analytic accuracy. Run the PRK functional/QA suite and the baseline, scheduling, exception and analytic-convergence probes before accepting this review. This is a verification gap, not a reproduced defect in the implementation.

  2. [P2] Reported downstream regression remains unresolved — .github/workflows/Downstream.yml:23. The supplied CI status identifies SciMLSensitivity/Core4 as a regression. I did not retrieve CI logs or reproduce it under this blind review's input restrictions, so I cannot attribute it to this diff or establish that it is pre-existing. The owning head must resolve it or provide a matching clean-default-branch comparison. Filing a deferred issue alone does not establish the required absence of test regressions.

Non-blocking findings

None identified from static inspection. The new equality assertion checks scheduling equivalence; no numerical formulas or existing assertions are changed.

What I ran

AI reviewer: codex-cli 0.164.0, model gpt-6.1-sol. The Session field gives the actual local session ID; no conversation URL is available.

  • git diff origin/master...HEAD, surrounding implementations, affected tests and repository test configuration; git diff --check origin/master...HEAD exited 0. Final git status --short was empty and HEAD matched the requested SHA. Neither root AGENTS.md nor root CLAUDE.md exists in this checkout; I used the supplied instructions.
  • git log -1 --format='%B': the newest commit contains no Agent-Harness or Agent-Model trailers, so both author attribution fields below are unknown.
  • julia --version: julia version 1.13.2.
  • uv run --locked python --version: Python 3.15.0, with the warning that --locked has no effect outside a project. No project-locked Python environment was verified.
  • A resource gate sampled Julia process trees, MemAvailable and /proc/loadavg, requiring fewer than five Julia roots, at least 16 GiB available memory and load below 32. It exited 75 after 3604 seconds, without launching Julia tests: Verification unavailable: resource limits did not permit a Julia test worker within one hour. The runtime's layered JULIA_DEPOT_PATH was preserved.

What I did not verify

The PRK functional and QA suites, existing convergence tests, and prepared targeted probes were not executed. Those probes cover both state representations, Bool/BaseThreads options, nested static/dynamic loops, exceptions, analytic fifth-order convergence, and the added regression test with the original helper restored only in memory. Dependency instantiation also remained unexecuted. I did not verify SciMLSensitivity/Core4 against the default branch, other supported Julia versions, or a locked dependency environment. Scratch preparation did not modify the checkout. No PR body, discussion, author rating or earlier review was inspected.

AUTHOR_GRADE: {"task":"bugfix","score":3,"difficulty":2,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"The diff is focused and adds a nested-threading regression test. Runtime correctness and failing-before/passing-after evidence were not verified because resource admission failed for one hour; the supplied downstream CI regression remains unresolved."}

Head link: 1c61b6f


🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol
Conversation: local session 01a1d257-53e3-7382-ac1e-956a412a6a16 (no session URL)

@singhharsh1708

Copy link
Copy Markdown
Member Author

The jl_in_threaded_region point is the one answered earlier in this thread, nothing new there. Core4 has been green on master since 11-04 and that run predates it, so I merged master to re-run.

This branch has not been deployed

No deployments
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