fix: resolve relative cache:// paths to absolute before spawning threads#72
Open
yannrichet-asnr wants to merge 1 commit into
Open
fix: resolve relative cache:// paths to absolute before spawning threads#72yannrichet-asnr wants to merge 1 commit into
yannrichet-asnr wants to merge 1 commit into
Conversation
run_local_calculation() calls os.chdir() in worker threads. Since CWD is
process-wide, a relative cache:// URI like cache://my-results/ can silently
fail: Path('my-results/').exists() returns False in any thread that
happens to run after another thread changed the CWD.
Fix: in fzr(), before handing calculators to run_cases_parallel(), resolve
any relative cache:// path against original_cwd so that all threads receive
an absolute URI and Path(...).exists() is CWD-independent.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a parallel-execution cache miss bug caused by process-wide os.chdir() calls in worker threads, by normalizing cache:// calculator paths to be CWD-independent before launching parallel work.
Changes:
- Normalize
cache://...calculator entries infzr()so relative cache paths become absolute (anchored tooriginal_cwd) beforerun_cases_parallel()spawns threads. - Add inline documentation explaining the CWD/threading race and why pre-resolution is needed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1497
to
+1504
| if calc.startswith("cache://"): | ||
| cache_rel = calc[8:] | ||
| cache_path = Path(cache_rel) | ||
| if not cache_path.is_absolute(): | ||
| cache_path = Path(original_cwd) / cache_path | ||
| resolved_calculators.append(f"cache://{cache_path.resolve()}") | ||
| else: | ||
| resolved_calculators.append(calc) |
Comment on lines
+1491
to
+1495
| # Resolve relative cache:// paths to absolute before spawning threads. | ||
| # run_local_calculation() calls os.chdir() in worker threads; since CWD is | ||
| # process-wide, a relative cache:// path resolved via Path(...).exists() in | ||
| # one thread may fail while another thread has changed the CWD. | ||
| resolved_calculators = [] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #71
Problem
run_local_calculation()callsos.chdir()inside worker threads. Since the CWD is process-wide, a relativecache://my-results/URI evaluated byPath(...).exists()in another thread will returnFalsewhenever the CWD has been changed — silently treating every cached case as a miss.Fix
In
fzr(), before handing the calculator list torun_cases_parallel(), resolve every relativecache://path to an absolute path anchored tooriginal_cwd:This is a single-line-of-user-impact change: absolute
cache://paths are unaffected, relative ones are normalised once on the main thread before anychdircan interfere.