Skip to content

Separate full_equations timeout from subprocess setup - #4845

Closed
ChrisRackauckas-Claude wants to merge 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-full-equations-timeout
Closed

Separate full_equations timeout from subprocess setup#4845
ChrisRackauckas-Claude wants to merge 2 commits into
SciML:masterfrom
ChrisRackauckas-Claude:agent/fix-full-equations-timeout

Conversation

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member

Depends on #4844. This branch is based directly on its head commit, 9d7b22a35bc107a880baa10b54b90cb41f561b3e; the only commit added here is 10949fa3552a3cd13aeb7f303d879b12f1a497ee.

Ignore this PR until it has been reviewed by @ChrisRackauckas.

Problem

The full_equations recursion regression test slept for 180 seconds before checking its child process. That combined package loading, cache-lock waits, precompilation, model setup, and the call being tested into one timeout.

On clean master, the child needed 265.37 seconds to finish after compiling ModelingToolkitBase. A contention-focused run spent 293 seconds precompiling before the tested call and then passed. The old assertion could therefore fail without full_equations hanging.

The subprocess itself is necessary: running the model against the parent of the original fix entered full_equations and was still running when an external 3600-second timeout terminated it.

Change

  • Touch a readiness marker after package loading, model construction, and mtkcompile.
  • Allow setup/cache work up to 3600 seconds.
  • Preserve the 180-second timeout specifically around full_equations.
  • Reap the child and require a successful exit, so a crash no longer satisfies the test.

Investigation

  • f215ec083a439be63603b83bc42178db24ce685e introduced the subprocess regression test with a 120-second combined timeout.
  • 9ed9aa758c759c8d49155679a915e8db0a88fcf1 increased that timeout to 180 seconds.
  • Clean-master GROUP=InterfaceI passed 1475 tests with 3 pre-existing broken tests in 49m03s, confirming that the reported failure is contention-dependent rather than deterministic.

Validation

  • Focused test, Julia 1.12: 4/4 passed in 10.8 seconds.
  • Focused test, Julia 1.10 LTS: 4/4 passed in 1m53.7s.
  • Cold/contention-focused test: 4/4 passed in 5m05.6s, including 293 seconds of child precompilation.
  • GROUP=InterfaceI julia +1.12 --project=lib/ModelingToolkitBase -e 'using Pkg; Pkg.test(; coverage = false)': 1498 passed, 5 pre-existing broken, 1503 total, in 38m16.8s.
  • Whole-repository Runic 1.5.1 check passed.

Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
Co-Authored-By: Chris Rackauckas <accounts@chrisrackauckas.com>
@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Stacked-PR plan/status:

  1. Merge prerequisite Fix SampleTime clock inference formatting #4844 (9d7b22a35bc107a880baa10b54b90cb41f561b3e).
  2. Review this PR's added commit 10949fa3552a3cd13aeb7f303d879b12f1a497ee, which only changes the full_equations subprocess test.
  3. Keep the setup/readiness wait separate from the 180-second execution timeout and retain subprocess isolation.
  4. Monitor CI; the focused Julia 1.12/LTS tests, full ModelingToolkitBase InterfaceI group, and whole-repository Runic check have passed locally.

GitHub does not allow the fork-only #4844 head branch to be selected as this upstream PR's base, so the PR targets master and explicitly depends on #4844.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

CI triage update (exact upstream-master comparison):

The required lib/ModelingToolkitBase [InterfaceI] jobs pass on Julia lts/current/pre. The remaining code-related failures are already present on exact clean upstream master commit 529c98930ee14191a0e90a32e37698ff6dc40309:

The PR MTKBase InterfaceII current/pre jobs are infrastructure failures: their check annotations say the self-hosted runner lost communication, and the test steps never completed. Job-specific rerun attempts were rejected because both available GitHub credentials lack Actions write permission. These issues are being kept out of this focused timeout PR.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Exact clean-master local confirmation for the MTSL downstream failure:

  • MTK/MTKBase: 529c98930ee14191a0e90a32e37698ff6dc40309 (detached clean upstream master)
  • ModelingToolkitStandardLibrary: c2ee11b86e873820521199f878e9d809f07e164a
  • Julia: 1.12.6
  • command: official GROUP=Core through Pkg.test(coverage = true)
  • result: exit 1, Core/test_analysis_points.jl had 37 passes / 1 error
  • error: test/test_analysis_points.jl:271 does not provide the loop-opening parameter feedback₊input2₊u(t) in the operating point

This exactly matches the upstream-master CI failure, so it is independent of this PR. The first local invocation spent most of its one-hour guard precompiling and timed out in sources.jl; I reran with the same resolved manifest and warmed isolated depot, and the official Core group then reached and reproduced the assertion above.

@ChrisRackauckas-Claude

Copy link
Copy Markdown
Member Author

Exact clean-master local confirmation for the Catalyst downstream failure:

  • MTK/MTKBase: 529c98930ee14191a0e90a32e37698ff6dc40309 (detached clean upstream master)
  • Catalyst: 53e6482dba3d884e4c40b7120d7d2356ec24ab9f
  • Julia: 1.12.6
  • command: official GROUP=Modeling through Pkg.test(coverage = true)
  • result: exit 1; Conservation Laws summary was 242 passes / 7 errors / 17 broken
  • all seven errors were unexpected passes at conservation_laws.jl lines 150, 152, 179, 183, 287, 290, and 325

This exactly matches the upstream-master CI failure, so it is independent of this PR. As with the MTSL audit, a first invocation exhausted its one-hour guard after first-time precompilation; the second invocation used the same resolved manifest and warmed isolated depot and reached the target failure through the official Modeling group.

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