fix: use pathlib.Path for cross-platform path parsing on Windows#6044
fix: use pathlib.Path for cross-platform path parsing on Windows#6044andrey-savov wants to merge 9 commits intoopen-edge-platform:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows path-handling issues in the native backend by switching from manual string splitting/joining to pathlib.Path-based path parsing and construction, preventing IndexError in otx find and improving cross-platform correctness.
Changes:
- Update
list_models()to derive the task directory viaPath(recipe).partsinstead ofrecipe.split("/"). - Update
OTXEngine.from_model_name()to build the target config path viaPath(default_config).parent / f"{model_name}.yaml"instead of split/join on/.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
library/src/otx/backend/native/engine.py |
Builds model config paths using Path(...).parent / ... to avoid platform-specific separators. |
library/src/otx/backend/native/cli/utils.py |
Uses Path(recipe).parts to extract the task segment in a platform-independent way for CLI table output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Tested end-to-end on Windows 11 with \ — now works fully. Also fixed a second Windows-only bug discovered during testing: \ when Rich tried to render the OTX logo through the legacy Windows console (cp1252 encoding can't handle the box-drawing characters). Fix: reconfigure stdout to UTF-8 at CLI entry and disable Rich's legacy Windows renderer (). |
- Replace recipe.split('/') with Path(recipe).parts in list_models() to
fix IndexError in 'otx find' on Windows (fixes open-edge-platform#4490)
- Replace string split/join in OTXEngine.from_model_name() with
Path.parent / name for cross-platform correctness
- Add None guard in from_model_name() with helpful error listing
supported tasks (addresses review comment)
- Cache Path(recipe) per loop iteration in list_models() (addresses
review comment)
- Reconfigure stdout to UTF-8 and disable Rich legacy Windows renderer
to fix UnicodeEncodeError when printing the OTX logo on Windows
- Update CHANGELOG
73e8281 to
4a4bedc
Compare
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…rey-savov/training_extensions into fix/windows-path-handling-issue-4490
|
Added a test case in est_from_config_with_model_name that passes an unsupported task to OTXEngine.from_model_name(), covering the previously missing ValueError branch. All 3 uncovered lines in �ngine.py now have coverage. Commit: 4f5581b. |
This reverts commit 5bafaff.
Summary
recipe.split("/")[-2]withPath(recipe).parts[-2]inlist_models()— fixesIndexErrorwhen runningotx findon Windows wherestr(Path)uses backslashesOTXEngine.from_model_name()withPath(default_config).parent / f"{model_name}.yaml"for the same reasonFixes #4490.
Test plan
uv run pytest library/tests/unit/backend/native/utils/test_api.py— 11 tests passuv run pytest library/tests/unit/backend/native/test_engine.py— 15 tests passotx find --task MULTI_CLASS_CLSon Windows no longer raisesIndexError