fix: tune.run() from concurrent threads corrupts shared runner state by AmirF194 · Pull Request #1610 · microsoft/FLAML

Amir Fathi (AmirF194) · 2026-09-22T23:55:32Z

Why are these changes needed?

flaml/tune/tune.py coordinates report()/run() through five module-level globals (_use_ray, _runner, _verbose, _running_trial, _training_iteration). Two threads calling tune.run() concurrently share all five: whichever thread's run() call most recently assigns _runner wins the global for every thread's subsequent report()/stop_trial() calls, including resetting it mid-flight while another thread is still running. sonichi's 2023 comment on this issue names the same root cause.

Fixed by moving that state into a threading.local subclass (_TuneState), so each thread gets its own runner/trial bookkeeping. report() and run() now read/write _state.<field> instead of the bare globals; the existing single-thread nested-reentrancy save/restore in run()'s try/finally is unchanged, just scoped per thread instead of process-wide.

Added a deterministic regression test (test/tune/test_concurrent_run.py): two threads are synchronized with events so thread A runs its own trial to completion, including its own finally-restore, while thread B is still paused mid-evaluation with its own runner already installed in the (then-shared) global. On current main this reproduces the exact crash reported here, AttributeError: 'NoneType' object has no attribute 'stop_trial', because thread A's finally resets the runner out from under thread B mid-flight. With the fix each thread keeps its own state and both trials complete with their own results.

Verification:

The new test reproduces the reported AttributeError on main and is green on this branch, on python 3.11.
test/tune/ passes (80 passed, 1 skipped); I skipped the ray- and xgboost-dependent files since I didn't have those installed.
pre-commit (black, ruff, pyupgrade, the repo's hooks) is clean on the changed files.
I haven't exercised the ray- and spark-using branches of run() myself, since I don't have ray/pyspark installed here; they get the same mechanical change (global to _state.<field>) as the branch the test covers.

Related issue number

Fixes #996

Checks

I've used pre-commit to lint the changes in this PR (note the same in integrated in our CI checks).
I've included any doc changes needed for https://microsoft.github.io/FLAML/. See https://microsoft.github.io/FLAML/docs/Contribute#documentation to build and test documentation locally.
I've added tests (if relevant) corresponding to the changes introduced in this PR.
I've made sure all auto checks have passed.

report()/run() coordinated through five module-level globals (_use_ray, _runner, _verbose, _running_trial, _training_iteration), shared by every thread. Two threads calling tune.run() concurrently overwrite each other's runner/trial bookkeeping mid-flight: whichever thread's run() call most recently assigns _runner wins the global for every thread's subsequent report()/stop_trial() calls, including resetting it while another thread is still running. Move that state into a threading.local subclass so each thread gets its own copy. The existing single-thread nested-reentrancy save/restore in run()'s try/finally is unchanged, just scoped per thread instead of process-wide. Fixes microsoft#996

Li Jiang (thinkall)

Overall review of the complete PR: changes are required.

flaml/tune/tune.py:45-65,209-229: tune.report() from a trainable-created callback/worker thread now sees a fresh thread-local state and loses the active runner. Use a propagatable logical run context and add a callback-thread reporting regression test.
flaml/tune/tune.py:512-537,963-964: concurrent runs still replace the same process-global logger handlers and level, causing log routing loss and leaving handlers corrupted after completion. Use run-scoped loggers/handlers and test simultaneous log files plus restoration.
flaml/tune/tune.py:509-685: state and logging are mutated before any encompassing cleanup block. Searcher, scheduler, or backend setup failures can leak state into nested/caught runs. Start one outer try/finally before the first mutation and restore all saved state for every failure path.

Posted by thinkall-agent-auto-reviewer

Scope the shared flaml.tune.logger handlers/level per run instead of replacing logger.handlers wholesale, so concurrent tune.run() calls no longer lose or leak each other's log routing (the logger was a sixth piece of process-global state left over after the five _TuneState fields were made thread-local). Wrap tune.run()'s setup (searcher/scheduler construction) in the same restore path used on normal return, so a setup failure no longer skips the state/logger restore and leaks mutated state into the next call on that thread. Add get_run_context()/use_run_context() so a trainable that spawns its own worker/callback thread can hand that thread the driving thread's run state before calling tune.report() from it, which the thread-local _TuneState broke silently.

Amir Fathi (AmirF194) · 2026-09-23T05:39:47Z

Pushed 7c19134, addressing all three points.

The logger was a sixth piece of shared state I'd missed: run() was still swapping the module-level logger's handlers/level wholesale. It now adds a per-run Handler with a run-scoped filter instead, plus reference-counted level tracking, so two concurrent runs never wipe out each other's handler or level.
Setup (searcher/scheduler construction) now runs inside a try/except that restores state and the logger before re-raising, so a setup failure no longer leaks mutated state into the next call on that thread. This was a pre-existing gap, not new to this PR, but worth closing while touching this code.
report() from a thread the trainable spawns itself is a real gap: thread-local state starts fresh there. Added get_run_context()/use_run_context() so a trainable can hand that thread its context explicitly; the report is still silently dropped if nobody does. Added a test asserting the drop is deterministic and the propagated path works, since the original report() docstring never documented cross-thread reporting as supported.

Three new tests cover each point (logging cross-contamination, setup-failure leak, worker-thread propagation), each verified to fail without the corresponding fix.

Copilot

Copilot review overview

🟡 Changes recommended

Helper-thread reporting regresses, propagated iteration state can become stale, and some backend failures still leak state.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3

Open (3)

Preserve runner context for helper-thread reports · New
Share report bookkeeping across worker and driving threads · New
Restore state and logging after all backend setup failures · New

What changed in this PR

Isolates concurrent tune.run() executions to prevent shared runner-state corruption.

Changes:

Replaces module globals with thread-local state.
Adds run-scoped logging and worker-thread context APIs.
Adds concurrency and state-restoration regression tests.

File	Description
`flaml/tune/tune.py`	Implements thread-local state, logging isolation, and context propagation.
`flaml/tune/__init__.py`	Exports the new context APIs.
`test/tune/test_concurrent_run.py`	Tests concurrent runs, logging, restoration, and worker reporting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    Being per-thread means a thread that a trainable spawns on its own (e.g.
+    a background worker that later calls tune.report()) starts from these
+    defaults too, with no runner attached; see get_run_context()/
+    use_run_context() below for the supported way to hand that thread the


+    _state.use_ray = ctx.use_ray
+    _state.runner = ctx.runner
+    _state.verbose = ctx.verbose
+    _state.running_trial = ctx.running_trial
+    _state.training_iteration = ctx.training_iteration


+    except Exception:
+        _restore_tune_state()
+        raise
+
+    if use_ray:


Li Jiang (thinkall)

Overall review of the complete current PR: changes are still required.

flaml/tune/tune.py:45-70: existing trainables that report from callback/helper threads silently lose results unless callers adopt the new context API. Preserve backward-compatible reporting automatically.
flaml/tune/tune.py:192: reused propagated contexts copy and discard training_iteration, causing duplicate report steps and potentially corrupting scheduler/searcher ordering. Keep bookkeeping in synchronized shared context state.
flaml/tune/tune.py:875,1034: Spark/backend initialization and sequential scheduler setup still occur outside the restoration guard. Failures leak thread state, handlers, and active logging bookkeeping. Enclose every setup and execution branch in one outer try/finally.

Posted by thinkall-agent-auto-reviewer

…safety fix report() from a trainable-spawned thread now works with no caller-side change: run() pins a contextvars context around each evaluation_function() call, and a patched threading.Thread inherits it into any thread the trainable spawns on its own. get_run_context()/use_run_context() stay as an explicit escape hatch for cases the patch can't reach. training_iteration moved off the per-thread/per-context snapshot onto a lock-guarded counter keyed by trial, so repeated handoffs for the same trial keep counting instead of restarting from a stale copy. Spark backend init and the sequential scheduler setup are now inside the same outer try/finally as the rest of run(), so a failure there restores _state/logger like every other setup failure does.

Amir Fathi (AmirF194) · 2026-09-23T07:39:34Z

Pushed 272ba31 for all three.

report() from a spawned thread now works without touching the trainable: run() pins the driving thread's context around each evaluation_function() call via a contextvars.ContextVar, and a small patch on threading.Thread.start()/run() carries that context into any thread the trainable spawns on its own. get_run_context()/use_run_context() are still there for cases the patch can't reach, like a persistent thread pool whose workers outlive a single submission.
training_iteration is off the per-thread snapshot now. It's a counter keyed on the trial itself (lock-guarded WeakKeyDictionary), so three separate worker threads handing off reports for the same trial actually see 0, 1, 2 instead of each restarting from the driving thread's stale copy. New test reproduces the duplicate-step bug directly.
Spark init (check_spark, the SparkSession) and the sequential scheduler setup are inside the same outer try/finally as everything else in run() now, so a failure there restores _state and the logger the same way a ray-path failure already did. Added a test that fails setup with use_spark=True (pyspark isn't installed here) and checks state afterward.

All three have new or rewritten tests in test/tune/test_concurrent_run.py, each one fails on the prior commit and passes now. Ran the full test/tune/ suite too: 91 passed, 1 skipped, one unrelated failure (test_nested_run needs lightgbm, not installed in my env, same failure on the prior commit).

…s with real pyspark installed The test relied on PySpark being absent so check_spark() would return unavailable. ubuntu-latest 3.11/3.12/3.13 CI legs install real pyspark (3.5.1/4.0.1/4.1.0), so check_spark() succeeds there and the test never raises ImportError. Patch check_spark() directly to force the failure path regardless of whether pyspark is actually installed.

Amir Fathi (AmirF194) · 2026-09-23T10:07:44Z

The new spark test was failing on the 3.11/3.12/3.13 legs: DID NOT RAISE ImportError. It assumed PySpark was absent so check_spark() would fail, but those legs install real pyspark (3.5.1/4.0.1/4.1.0), so check_spark() succeeded and the test never hit the ImportError path.

Patched check_spark() itself with mock.patch so the setup-failure path is forced regardless of whether pyspark is actually installed. Verified in Docker: all 6 tests in test_concurrent_run.py pass both with real pyspark installed and without it. Negative control, reverting tune.py to the pre-fix commit with this test kept: 3 of 6 fail, including this one, with the same leaked-handler assertion CI was hitting.

Li Jiang (thinkall)

Overall review of the complete current PR: changes are still required.

flaml/tune/tune.py:207-214,456-484: context is captured per worker-thread lifetime. Pre-existing workers receive none, and reused workers remain pinned to the first trial, dropping or misrouting later reports. Propagate context per submitted callback/task and test pre-warmed and cross-trial executor reuse.
flaml/tune/tune.py:204-218: Thread subclasses overriding run() bypass the global base-method patch and silently lose tune.report(). Add a propagation design and regression that supports existing subclasses.
flaml/tune/tune.py:183-221: importing FLAML globally monkey-patches threading.Thread and copies every ambient application ContextVar into new threads. Propagate only FLAML-owned state through a scoped mechanism.
flaml/tune/tune.py:101-102: automatically propagated callback threads lack the owning log-run ID, so their log records are filtered out of the run log. Include run-scoped logging identity in the propagated context.

Posted by thinkall-agent-auto-reviewer

…afety fix Three gaps in the automatic context-propagation patch, all real: 1. ThreadPoolExecutor worker threads call Thread.start() once, when the pool spins them up, not once per submitted task, so a task submitted to an already-warmed-up worker never saw a context captured at start() time. Fixed by also patching ThreadPoolExecutor.submit() to attach the context per task rather than per thread; verified this is not otherwise handled (a plain contextvars.ContextVar is not propagated into an already-running pool worker either). 2. The patch replaced threading.Thread.run directly, which a Thread subclass overriding its own run() shadows. Now wraps Thread._bootstrap_inner instead, which CPython calls internally and which invokes self.run() regardless of which run() that resolves to. 3. The patch captured contextvars.copy_context() (the full ambient Context), dragging every application ContextVar into every new thread process-wide. Now captures and restores only FLAML's own _RunContext value. Folded log_run_id into _RunContext and propagate it through both paths too: a worker thread's log records were being filtered out of the run log the same way an un-propagated report() used to be dropped, since the run-scoped log filter checks a separate thread-local the context patch was not setting. Five new regression tests: pre-warmed executor worker, pre-warmed executor reused across two trials, a Thread subclass overriding run(), and log records reaching the run log through both the plain-thread and executor paths. All five fail on the prior commit and pass with this fix; the six pre-existing concurrency tests are unaffected.

Amir Fathi (AmirF194) · 2026-09-23T11:45:55Z

Pushed 16773f5 for all four.

ThreadPoolExecutor.submit() to an already-warmed-up worker doesn't see a context captured at Thread.start() time, since that worker's start() ran once, before this task existed. Now also patching ThreadPoolExecutor.submit() to attach the context per submitted task. Verified a plain contextvars.ContextVar isn't propagated into a pre-warmed worker either, so the Thread patch alone could never reach this case.
Now wrapping Thread._bootstrap_inner instead of Thread.run: CPython calls it internally and it invokes self.run() regardless of which run() a subclass overrides.
Capture and restore only the one _RunContext value now, never contextvars.copy_context(). That was the whole ambient Context, not just ours.
Folded log_run_id into _RunContext and propagate it through both paths. The run-scoped log filter checks a separate thread-local the patch was never setting, so a worker thread's log records were filtered out of the run log the same way an un-propagated report() used to be dropped.

Five new tests: pre-warmed executor worker, the same reused across two trials, a Thread subclass overriding run(), and log records reaching the run log through both the plain-thread and executor paths. All five fail on the prior commit and pass with this one; the six existing concurrency tests are unaffected, and a full test/tune/ run shows the same 17 pre-existing failures (missing optuna, order-dependent) with or without this diff.

Li Jiang (thinkall)

Overall review of the complete current PR: changes are still required.

flaml/tune/tune.py:229-230,278-293,545-551: generic persistent callback/queue workers started before tune.run() still receive no context, so reports and run-scoped logs are silently lost. Preserve automatic single-run compatibility when ownership is unambiguous and add a pre-started queue-worker regression.
flaml/tune/tune.py:233-242,283-291,574-587: propagated contexts remain usable after their trial finishes. Late threads or executor work can rewrite completed trial results and raise StopIteration. Make contexts lifecycle-aware, invalidate them at trial completion, reject inactive/finished contexts before processing, and avoid retaining trial context as a pool worker's thread-lifetime default.

Posted by thinkall-agent-auto-reviewer

…follow-up) thinkall's fourth CHANGES_REQUESTED review, point 2: a _RunContext captured for a trial stays usable after that trial finishes. A worker thread or executor task that captured get_run_context() and reports late, after the trial is already TERMINATED, still wrote through: process_trial_result() overwrote the trial's final metric_analysis and last_result with the stale value, and report()'s own trailing `if trial.is_finished(): raise StopIteration` (the normal scheduler-stop signal for the current report) then raised into the late caller too, which has no reason to expect it the way a trainable's own control-flow loop does. report() now checks trial.is_finished() before processing and drops the late report instead. New regression test captures a context, lets the trial finish, then reports through the stale context: fails with an uncaught StopIteration on the prior commit, passes now, and asserts last_result/metric_analysis are byte-for-byte unchanged by the late write. Point 1 (a persistent callback/queue worker started before tune.run() never gets context for later items) is not fixed here: replied on the PR with why an automatic fallback is not safe to ship.

Amir Fathi (AmirF194) · 2026-09-27T14:47:56Z

Pushed 04463eb for point 2, and I'm not patching point 1, here's why.

Point 2: report() now checks trial.is_finished() before processing and drops the report if so. A stale context still writing through would corrupt the trial's already-final metric_analysis via process_trial_result(), and the trailing is_finished() check (the normal scheduler-stop signal for the CURRENT report) would then raise StopIteration into a caller that never expected it. New test captures a context, lets the trial finish, reports through the stale context: uncaught StopIteration on the prior commit, clean no-op now, last_result/metric_analysis asserted unchanged.

Point 1: traced this all the way through and I don't think an automatic fallback is safe. The only information available to disambiguate a context-less report is "how many tune.run() calls are active right now", so the only possible fallback is "if exactly one, assume it's that one's current trial." The problem: it reads runner.running_trial live, which SequentialTrialRunner reassigns to a new Trial object every step(). A report queued while trial A was running but delivered after the runner has moved on to trial B lands on B, which is_finished() won't catch since B is genuinely still running. That trades a silent drop for a silent wrong-trial write, worse for a thread-safety fix than the current behavior. Fixing it for real needs the queue item itself to carry get_run_context() from enqueue time, which only the worker's own code can do, so I think this is the architectural limit you flagged rather than something to patch around.

Verified in Docker (python:3.11-slim): fails before / passes after on the new test, full test/tune/test_concurrent_run.py (12/12) and test/tune/ minus test_tune.py (needs xgboost, same gap as earlier rounds) both green, black and ruff clean.

Li Jiang (thinkall)

Overall review of the complete current PR: changes are still required.

flaml/tune/tune.py:230-239,545-551: generic pre-started or cross-trial persistent queue workers still lack per-dispatch context, so reports/logs are lost or remain pinned to an earlier trial. Add lifecycle-aware per-dispatch propagation or an unambiguous compatibility route, with pre-started and reused queue-worker tests.
flaml/tune/tune.py:577-596: trial.is_finished() and process_trial_result() are not atomic. Trial completion can occur between them, allowing a stale report to rewrite a completed result and raise StopIteration. Synchronize report admission with lifecycle invalidation and add a barrier-based race regression.

Posted by thinkall-agent-auto-reviewer

…follow-up) thinkall's fifth CHANGES_REQUESTED review, point 2: trial.is_finished() and runner.process_trial_result() were not atomic. A second, truly concurrent report for the same trial (a trainable's own worker threads reporting for the trial they share, for instance) could pass the is_finished() check while the trial was still running, and if the FIRST report's scheduler decision finished the trial before the second one reached process_trial_result(), the second one still wrote through, silently replacing the trial's real final result with a stale one. Added a per-trial admission lock and moved the actual admission decision (the check process_trial_result() acts on) inside it, so the re-check and the write are now one atomic step. The existing fast-path check right after resolving the trial is unchanged; it just stops being the only line of defense. New test forces the exact interleaving deterministically (events, not a sleep): worker B is paused, via a patched _next_training_iteration, right after its own is_finished() check returns False, until worker A's entire report() call, including the scheduler decision that terminates the trial, has completed. Verified this reproduces the bug on the prior commit (B's stale report wins, 2.0 instead of A's 1.0) and is fixed here. Point 1 (a persistent queue worker started before tune.run() lacking per-dispatch context) is restated from the fourth review. Automatic propagation for it is still not being added, for the reason already given: nothing at report()-call time can reconstruct which trial a delayed item was produced for. The compatibility route the review also names already exists (get_run_context()/use_run_context()); added a test exercising it on exactly the shape described, a worker thread started before tune.run() and reused across two trials.

Amir Fathi (AmirF194) · 2026-09-27T21:34:49Z

Point 2: real race, fixed. is_finished() and process_trial_result() weren't atomic: a concurrent report for the same trial could pass the check while it was still running, then have another report's scheduler decision finish the trial before it reached process_trial_result(), and it still wrote through. Added a per-trial lock and moved the actual admission check inside it, so the re-check and the write are one step now. New test forces the interleaving with events, not a sleep: worker B is paused right after its own is_finished() check (via a patched _next_training_iteration) until worker A's report has fully terminated the trial. Confirmed it fails on the prior commit (B's stale 2.0 wins over A's 1.0) and passes with the fix.

Point 1: not adding automatic propagation, same reason as before and still true after tracing it again: nothing at report()-call time can tell which trial a delayed queue item was produced for, since the runner's running_trial has already moved on by the time it's dequeued. The compatibility route you're also asking for already exists: get_run_context()/use_run_context(), captured by the producer and carried on the queue item itself. Added a test for exactly the shape described, a worker started before tune.run() and reused across two trials, and it works.

The failing build (ubuntu-latest, 3.12) leg is a yaml.parser.ParserError reading a corrupted mlruns/.../meta.yaml, inside FLAML's own mlflow autologging integration (flaml/fabric/mlflow.py), not this diff. It's the only one of 11 legs to fail on this commit; 3.10, 3.11, 3.13 and every Windows/ARM leg passed with identical code. The concurrent-run test that touches it has been in this PR since the first commit. Looks like a pre-existing race in mlflow's file-based run store when two tune.run() calls log into the same experiment at once, unrelated to the module-globals fix here. Leaving that one alone, it's a different subsystem.

Li Jiang (thinkall)

Overall review of the complete current PR: changes are still required.

flaml/tune/tune.py:631 and flaml/tune/trial_runner.py:96-102: report admission and stop_trial() do not share the same lifecycle lock, so a late report can still mutate a terminated trial.
flaml/tune/tune.py:581-601,102-103: generic pre-started queue workers still lose reports and run-scoped logs unless callers adopt the new explicit context API, breaking backward compatibility.
flaml/tune/tune.py:625-641: training iterations are allocated before locked admission, allowing concurrent reports to reach schedulers in reverse order. Allocate iterations inside the admitted critical section.
flaml/tune/tune.py:122-130: restoration saves the effective logger level but restores it as an explicit level, permanently changing inherited logging behavior. Restore the exact original logger.level.

Posted by thinkall-agent-auto-reviewer

…afety fix thinkall's sixth review, three real gaps fixed: 1. stop_trial() and report()'s admission did not share a lock. A straggling background-thread report could still be mutating trial.last_result/status via process_trial_result() at the same instant run()'s own loop called stop_trial() on the same trial. stop_trial() now takes the same per-trial _admission_lock_for report() already used. 2. training_iteration was allocated before the admission lock was acquired, so two concurrent reports for one trial could be handed iterations in one order and reach process_trial_result() in the other. Allocation moved inside the same critical section as admission. 3. _logger_level_enter() saved logger.getEffectiveLevel() (the resolved level, walking up the logger hierarchy) instead of logger.level (the logger's own, possibly NOTSET, level), so a logger that was inheriting ended up pinned to an explicit level after every run finished. Restores logger.level now. The review's fourth point, automatic context propagation for a generic pre-started queue worker, is not implemented for the reason given in the third and fourth review rounds: report() cannot tell which trial a delayed queue item belongs to once the runner has moved on to a later trial, and a fallback that guesses would silently attribute a report to the wrong trial, worse than today's silent drop. The documented get_run_context()/use_run_context() API already covers this case explicitly; added a test for the specific pre-started, reused-across-trials shape the review describes. Five new regression tests, each with a negative control confirming it fails on the pre-fix code and passes on this commit. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>

Amir Fathi (AmirF194) · 2026-09-28T03:17:52Z

Pushed 0a055a6 for points 1, 3 and 4.

stop_trial() now takes the same per-trial lock report()'s admission uses, so it can no longer run at the same instant a straggling report is still inside process_trial_result() for that trial.
training_iteration is now allocated inside the admission lock, immediately before the write it orders, so two concurrent reports can no longer be handed iterations in one order and admitted in the other.
The saved logger level is now logger.level (the logger's own, possibly NOTSET, value), not getEffectiveLevel() (the resolved one), so a logger that was inheriting goes back to inheriting once every active run exits instead of being pinned to whatever it resolved to during the run.

Point 2 is the same conclusion as the last two rounds, re-traced rather than repeated: report()'s only handle on which trial a delayed queue item belongs to is whatever context the producer captured, and the runner reassigns running_trial on every step, so any fallback that resolves the trial live at dequeue time can attribute a late item to the wrong trial. That is worse than the current silent drop. get_run_context()/use_run_context() already covers this; added a test for the specific pre-started, cross-trial-reused worker shape here.

Five new regression tests, each with a negative control against the pre-fix code.

Li Jiang (thinkall)

Overall review of the current PR: changes are still required.

The latest commit fixes the report/stop lifecycle lock, iteration ordering, and exact logger-level restoration. One prior backward-compatibility blocker remains: flaml/tune/tune.py:247-263,267-313,595-620 only propagates a run context to threads started during evaluation or tasks submitted through ThreadPoolExecutor. A generic queue/callback worker started before tune.run() has neither context nor a runner, so its existing tune.report() calls silently return without recording the metric, and its log records are filtered out at tune.py:102-103. The new pre-started-worker test changes the caller to pass get_run_context()/use_run_context() rather than testing unchanged callers.

Please preserve legacy reporting/logging when ownership can be determined safely, and do not silently discard an ambiguous context-less report. Add regressions with an unchanged pre-started queue worker and a reused worker across trials. If that compatibility cannot be preserved, document and explicitly signal the unsupported case rather than presenting it as compatible.

Posted by thinkall-agent-auto-reviewer

…eventh review) A generic queue/callback worker started before tune.run() has neither a propagated context nor a runner, so its tune.report() calls silently returned without recording the metric. Automatic attribution is still not added (SequentialTrialRunner.step() reassigns running_trial every step, so a live-resolved fallback would attribute a delayed report to whichever trial happens to be running by the time it is handled, not the one it was produced for) but the drop is no longer silent: it logs once, and the record bypasses the run-scoped filter so it reaches the active run's own log file instead of being swallowed by it. New test starts a legacy queue worker before tune.run(), an unchanged caller with no get_run_context()/use_run_context(), and asserts the trial's result stays untouched (None) while the warning appears exactly once in the run's log file. Fails on the prior commit with an AttributeError (no _context_less_report_warned to patch), passes on this one. Verified in Docker (python:3.11-slim): test/tune/test_concurrent_run.py 18/18 pass; test/tune/ minus test_tune.py (needs xgboost, pre-existing gap) 98 passed, 1 skipped; black 23.3.0 and ruff 0.0.261 clean. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>

Amir Fathi (AmirF194) · 2026-09-28T07:00:43Z

Pushed 865bd8c.

Not adding automatic attribution, same conclusion as the last several rounds and unchanged after re-reading the ask: nothing available at report() time can safely resolve which trial a delayed queue item belongs to, since the runner reassigns running_trial every step.

What changed: the drop is no longer silent, per the "document and explicitly signal" suggestion. report() now logs a one-time warning when it drops a context-less report, and the record carries a flag that bypasses the run-scoped log filter, so it reaches the active run's own log file instead of being swallowed the same way the report itself was. get_run_context()/use_run_context() is named in the message as the fix.

New test starts a legacy queue worker before tune.run() with no get_run_context()/use_run_context() call at all, the unchanged-caller shape described above. It asserts the trial's result stays None (not corrupted) and the warning appears exactly once in the run's own log file. Verified in Docker: fails on the prior commit with an AttributeError (nothing to patch pre-fix), passes on this one. test/tune/test_concurrent_run.py 18/18, test/tune/ minus test_tune.py (needs xgboost, pre-existing gap) 98 passed/1 skipped, black and ruff clean.

Li Jiang (thinkall)

Overall review of the current PR: changes are still required.

flaml/tune/tune.py:247-313,642-724: unchanged queue/callback workers started before tune.run() still cannot see the run context. Their reports remain unrecorded and their log records are filtered out. The new regression test explicitly asserts last_result is None, codifying the backward-incompatible data loss instead of preserving the earlier working caller. Preserve reporting when work-item ownership can be propagated; for ambiguous ownership, fail explicitly rather than silently treating a lost metric as a successful trial.
flaml/tune/tune.py:565-598: the diagnostic is gated once per process. After the first affected run, another run can drop metrics without any warning. Signal each affected run/call so later data loss is visible.
flaml/tune/tune.py:102-103: the flaml_tune_unscoped bypass sends an unattributed warning to every concurrently active run handler, placing unrelated diagnostics in the wrong run logs. Keep unattributed warnings separate from run-owned log files.

The current Ubuntu Python 3.12 build check is also failing (pytest timed out); the other completed legs and formatting pass.

Posted by thinkall-agent-auto-reviewer

…oft#996 eighth review)

Amir Fathi (AmirF194) · 2026-09-30T10:32:47Z

Pushed 2876ab5 addressing all three points.

A context-less report now resolves automatically when exactly one tune.run() is active anywhere in the process (the pre-Parallel execution of model training throws StopIteration Exception #996 module-global design's own fallback), instead of dropping the report outright. Two or more concurrent runs still cannot be attributed safely, so that case now raises RuntimeError rather than guessing or silently dropping.
The one-time warning is reset per run() call, not once per process, so a later run hitting the same unsupported shape (no active run, none of the above) warns again.
Moved the warning off the shared logger onto a new flaml.tune.diagnostics logger that run() never attaches a run-owned handler to, so it can no longer land in another concurrently active run's own log file. Dropped the flaml_tune_unscoped filter bypass this replaced.

test/tune/test_concurrent_run.py: 21/21 pass, including two new tests for the single-run fallback and the multi-run RuntimeError. Positive control: reverting just _resolve_unambiguous_active_runner's new lookup back to always returning None fails the fallback test with the pre-fix drop, confirming it's pinned to that hunk. Broader test/tune/ (minus test_tune.py, needs xgboost): 101 passed, 1 skipped, same as prior rounds. black/ruff clean on both files. The Ubuntu 3.12 timeout from the last round is unrelated to this change, already written up in the state history for whoever answers it next.

Li Jiang (thinkall)

Overall review of the current PR: one compatibility issue remains.

flaml/tune/tune.py:114-115,794-807,895-897: the single-active-run fallback now restores tune.report() for an unchanged pre-started queue worker, but it sets _state.log_run_id only for the duration of that report call. The same worker has no log identity when it emits ordinary logger.info(...) before or after tune.report(), so _RunScopedFilter drops those records from the sole run’s configured log file. The pre-PR process-global handler recorded them, and the new legacy-worker regression checks metrics but no longer checks a worker log marker. Preserve run-scoped logging for this unambiguous single-run worker case (without attributing records when multiple runs are active), and add a regression with the unchanged pre-started queue worker emitting a log marker outside its report call.

The new commit addresses the previously lost metric, once-per-process warning, and cross-run warning broadcast. Current checks pass; no other blocking finding from this delta.

Posted by thinkall-agent-auto-reviewer

Li Jiang (thinkall) requested changes Sep 23, 2026

View reviewed changes

Li Jiang (thinkall) requested a balanced review from Copilot September 23, 2026 05:45

Copilot started reviewing on behalf of Li Jiang (thinkall) September 23, 2026 05:47 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Merge branch 'main' into fix/996-tune-run-thread-safety

8b62086

Li Jiang (thinkall) requested changes Sep 23, 2026

View reviewed changes

Li Jiang (thinkall) requested changes Sep 27, 2026

View reviewed changes

Li Jiang (thinkall) requested changes Sep 28, 2026

View reviewed changes

fix: preserve legacy worker reporting and isolate diagnostics (micros…

2876ab5

…oft#996 eighth review)

Li Jiang (thinkall) requested changes Sep 30, 2026

View reviewed changes

Conversation

Amir Fathi (AmirF194) commented Sep 22, 2026

Why are these changes needed?

Related issue number

Checks

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 23, 2026

Uh oh!

Amir Fathi (AmirF194) commented Sep 23, 2026

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 23, 2026

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 27, 2026

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 27, 2026

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 28, 2026

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 28, 2026

Uh oh!

Li Jiang (thinkall) left a comment

Choose a reason for hiding this comment

Uh oh!

Amir Fathi (AmirF194) commented Sep 30, 2026

Uh oh!

Li Jiang (thinkall) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Li Jiang (thinkall) left a comment •

edited

Loading