Skip to content

fix: escape Windows backslashes in Julia path strings - #85

Open
ghita-el-amlaqui wants to merge 2 commits into
gridfm:mainfrom
ghita-el-amlaqui:fix/windows-path-julia-unicode-escape
Open

ghita-el-amlaqui wants to merge 2 commits into
gridfm:mainfrom
ghita-el-amlaqui:fix/windows-path-julia-unicode-escape

Conversation

@ghita-el-amlaqui

@ghita-el-amlaqui ghita-el-amlaqui commented Sep 21, 2026 •

Copy link
Copy Markdown

Fix: Escape Windows backslashes in Julia path strings

Problem

Running gridfm_datakit generate on Windows fails with a Julia ParseError:

ERROR: ParseError:
# Error @ none:3:34
data = PowerModels.parse_file("C:\Users\...")
#                              └┘ ── invalid unicode escape sequence

Windows paths embedded directly into Julia source code as f-string literals are parsed by Julia as containing invalid Unicode escape sequences (e.g. \U, \A), crashing before any OPF solve can begin.

Fix

Normalise both paths to forward slashes before constructing the Julia code snippet:

jl_network_path = str(network_path).replace("\\", "/")
jl_tmp_path = str(tmp_path).replace("\\", "/")

Julia and PowerModels accept forward-slash paths on Windows. Behaviour on Linux/macOS is unchanged.

Files changed

File Change
gridfm_datakit/network.py Convert paths to forward slashes before embedding in Julia string literals

Type of change

  • Bug fix (non-breaking)

@albanpuech

albanpuech commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Thanks! Could you please also run the pytests on your Windows machine and check that they all pass?

pip install -e ".[dev,test]"
pytest tests/ -n <number_of_cores_of_your_machine> -v

@romeokienzler

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Thanks for the fix — normalising the paths to forward slashes before embedding them in the Julia snippet is a clean, targeted solution, and it's nicely contained (one file, clear comment).

What's needed

  • The one open item is @albanpuech's request above: since CI runs on Linux it can't exercise the Windows path this actually fixes, so please run the tests on your Windows machine and confirm they pass:
    pip install -e ".[dev,test]"
    pytest tests/ -n <cores> -v
  • Optional, not blocking: a small regression test asserting the generated Julia lines contain no backslashes (e.g. feeding a C:\\Users\\... style path) would lock in the behaviour, per the CONTRIBUTING testing checklist.

Everything else looks good — all checks are green and the branch is clean, so no rebase needed. I'll leave the merge call to a maintainer.

— 🤖 _automated pre-review; a maintainer will follow up_

@romeokienzler

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Quick follow-up — main was merged into your branch, so I re-checked the state. Your fix is unchanged and still looks good; nothing new was introduced by the merge.

Two things:

  • New blocker: the PR now shows as BLOCKED because the DCO check is failing. DCO requires every commit to carry a Signed-off-by: line. The failing commit is the main→branch merge commit (not authored by you), so this likely isn't on your original change — a maintainer may just need to re-do that merge with git merge -s (sign-off) or the DCO can be remediated per the linked instructions. Worth a maintainer's eyes.
  • Still open from before: @albanpuech's request to run pytest tests/ -n <cores> -v on your Windows machine and confirm they pass, since CI (Linux) can't exercise the Windows path this fixes. The optional regression test asserting no backslashes in the generated Julia lines is still nice-to-have, not blocking.

I'll leave the merge/DCO remediation call to a maintainer.

— 🤖 _automated pre-review; a maintainer will follow up_

@albanpuech

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Thanks! Could you please also run the pytests on your Windows machine and check that they all pass?

pip install -e ".[dev,test]"
pytest tests/ -n <number_of_cores_of_your_machine> -v

Hello @ghita-el-amlaqui , could you please check this? thank you!

@romeokienzler

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Quick update — good news since my last note: the DCO check is now passing, so the BLOCKED state I flagged earlier has cleared. The remaining CI checks are green except the pytests, which are still running.

So the one thing @albanpuech is (rightly) waiting on is the Windows-side confirmation — since CI only runs on Linux, it can't exercise the Windows path your change actually fixes:

pip install -e ".[dev,test]"
pytest tests/ -n <cores> -v

A short paste of the passing summary would let a maintainer wrap this up. (The optional no-backslash regression test is still just nice-to-have, not blocking.)

— 🤖 _automated pre-review; a maintainer will follow up_

@ghita-el-amlaqui

Copy link
Copy Markdown
Author

Hello @romeokienzler, here's a recap of the tests I ran on Windows:

  1. pip install -e ".[dev,test]" : Successfully ran with no errors.

  2. pytest tests/ -n <cores> -v : 2 failed, 239 passed, 360 skipped, 34 warnings (duration: 1h 25min 40s)

Error messages:

FAILED tests/test_all_perturbations.py::test_all_perturbation_combinations - AssertionError: 13 perturbation combinations failed
FAILED tests/test_solver_output.py::TestRedirectCStdio::test_redirects_c_level_writes_to_file - TypeError: argument of type 'NoneType' is not iterable

I'll include the fixed code in the new PR.

@romeokienzler

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Thanks for running the full suite on Windows and pasting the summary — that's exactly the confirmation we needed.

The key point for this PR: the Windows parse that your change fixes now works end-to-end (install succeeded, 239 passed), so the backslash→forward-slash normalisation does its job.

On the two failures — neither looks related to your diff:

  • test_solver_output.py::...test_redirects_c_level_writes_to_file exercises fd-level dup2 stdio redirection (gridfm_datakit/process/solver_output.py), which behaves differently on Windows — a pre-existing platform issue, not your path change.
  • test_all_perturbations.py::test_all_perturbation_combinations is unrelated to Julia path escaping too.

So I wouldn't hold this PR on them. Fixing those Windows-specific test issues in a separate PR (as you suggested) is the right call — keeps this fix small and reviewable.

One housekeeping item: the branch now shows as BEHIND main, so a quick rebase/merge of latest main before merge would be good.

I'll leave the merge decision to a maintainer.

— 🤖 _automated pre-review; a maintainer will follow up_

@ghita-el-amlaqui
ghita-el-amlaqui force-pushed the fix/windows-path-julia-unicode-escape branch from 6c8bb8d to 6f1342f Compare October 6, 2026 21:40
Signed-off-by: ghita-el-amlaqui <ghita.el.amlaqui@ibm.com>
@romeokienzler

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Thanks for pushing the fixes — and for signing off the commit (DCO is green).

Checking the new commit (6f1342f) against the two Windows failures you reported:

  • test_all_perturbations — the SHA-1 short-hash directory name is a tidy way around the Windows MAX_PATH (260-char) limit, and it's test-only so it doesn't touch the shipped fix. Good.
  • test_solver_output::test_redirects_c_level_writes_to_file — the skipif(os.name == 'nt', ...) cleanly sidesteps ctypes.CDLL(None) not loading libc on Windows; imports (os, pytest) are already in place. Good.

Both are isolated to tests/ and don't alter the backslash→forward-slash normalisation, so the core fix stays untouched.

One small note: I'd earlier suggested landing these in a separate PR to keep this one minimal — but folding them in here is a perfectly reasonable call too, since they're small and directly unblock your Windows run. Entirely your preference; I don't think it's worth splitting now.

If you can re-run pytest tests/ -n <cores> -v on Windows once more to confirm those two now pass (or skip), that would close the loop. Branch state looks fine (no rebase flagged). Merge call stays with a maintainer.

— 🤖 _automated pre-review; a maintainer will follow up_

@ghita-el-amlaqui

Copy link
Copy Markdown
Author

Fix: Resolve two Windows-specific test failures

This PR fixes also the two test failures that only manifest on Windows:

1. test_all_perturbation_combinations — WinError 206 (path too long)

File: tests/test_all_perturbations.py

The test used the full perturbation combination string (e.g. load_agg_load_profile_topo_none_gen_cost_permutation_adm_random_perturbation_mode_opf) as a directory name for generated output. On Windows, the total path — workspace root + test subdir + case name + Parquet partition suffix — exceeded the 260-character MAX_PATH limit, causing Arrow/PyArrow to fail when creating or opening partition directories.

Fix: Replace the long combination name with a 12-character SHA-1 hash as the on-disk directory name. The full human-readable name is preserved for test result reporting.


2. test_redirects_c_level_writes_to_file — TypeError: argument of type 'NoneType' is not iterable

File: tests/test_solver_output.py

ctypes.CDLL(None) is a POSIX convention to load the C standard library. On Windows, None is not a valid DLL argument — ctypes crashes before the DLL is even loaded, with a TypeError when it tries to check whether None contains path separators.

Fix: Add @pytest.mark.skipif(os.name == "nt", ...) to skip this test on Windows. The production code already has a _LIBC = None fallback for platforms where libc is not loadable; the test was simply missing the matching guard.


Testing

Both fixes were validated by running the full test suite:

pytest tests/ -n auto -v

Previously: 2 failed, 239 passed, 360 skipped
After fix: 0 failed, 239 passed, 361 skipped (+1 from the newly skipped Windows-incompatible test)

@romeokienzler

Copy link
Copy Markdown
Collaborator

@ghita-el-amlaqui Perfect — that closes the loop. The re-run on Windows going from 2 failed → 0 failed (239 passed, 361 skipped) confirms both test fixes do their job: the SHA-1 short-hash dir name dodges the MAX_PATH limit, and the skipif(os.name == "nt") cleanly skips the libc-dependent redirect test.

So from a pre-review standpoint this now looks complete:

  • The core fix (backslash→forward-slash path normalisation) is verified end-to-end on Windows.
  • The two previously-failing Windows tests are resolved, isolated to tests/, and don't touch the shipped change.
  • DCO is green; no rebase flagged.

Looks ready for a maintainer's look. Merge call stays with a maintainer.

— 🤖 _automated pre-review; a maintainer will follow up_

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.

3 participants