fix: resolve infinite loop in _find_config on Windows systems #4970

Julfried · 2024-12-17T16:08:37Z

Issue #4967

Description of changes:
This PR fixes an infinite loop in _find_config that occurs on Windows systems. The issue was caused by using Path.match("/") to detect root directories, which never matches on Windows due to different path separators.

Changes made:

Replace platform-specific Path.match("/") with Path.anchor comparison
Implement proper root directory detection for both Windows and Unix systems
Add comprehensive cross-platform tests

Testing done:

Verified fix on both Windows and Linux systems
Added test_find_config_cross_platform to validate:
- Basic path traversal
- Nested directory structures
- Root directory handling
- Non-existent config scenarios
Added test_find_config_path_separators to verify handling of:
- Forward slashes
- Backslashes
- Mixed separators
All existing tests pass without modification

Merge Checklist

General

I have read the CONTRIBUTING doc
I certify that the changes I am introducing will be backward compatible
I used the commit message format described in CONTRIBUTING
I have passed the region in to all S3 and STS clients (N/A for this change)
I have updated necessary documentation (N/A - internal implementation change)

Tests

I have added tests that prove my fix is effective
I have added unit tests to ensure backward compatibility
I have checked that my tests are not configured for a specific region or account
I have used unique_name_from_base for resource names (N/A - unit tests only)
No new dependencies added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Julfried · 2024-12-21T12:48:36Z

Quick update: I was unsure about two checklist items and checked them with N/A, because I think they are not applicable for my changes:

"Passed region to S3/STS clients"
"Documentation updated"

Please let me know if there is anything I need to do, to get this merged. Thanks!

ahmednmourad · 2024-12-31T10:52:01Z

I have been struggling with this for days. Thank you!

Julfried · 2024-12-31T11:15:19Z

Thanks, hopefully they merge this soon, because this bug is really confusing and frustrating. Especially for new users!

Julfried · 2025-01-16T19:30:16Z

Hey @nargokul

Just following up on this PR! The fix resolves a frustrating infinite loop issue on Windows (#4967) caused by a root path detection method that only worked on Linux. It updates the logic to properly handle cross plattform paths.

Looks like the CI is stuck on the wait-for-approval step—would you mind approving it so the checks can run? Let me know if there’s anything else I should adjust.

Thanks!

Julfried · 2025-01-27T14:55:54Z

Since this PR seems to be stale, I’d like to follow up to see if there’s anything else needed from my side to move it forward.

@benieric, @pintaoz-aws – Would you be able to take a look at this? I’ve recently rebased the branch to the latest master to ensure it’s up-to-date and ready for review.

This fix addresses a frustrating bug (#4967) that causes an infinite loop on Windows systems. Resolving this would greatly improve the developer experience, especially for new users facing this issue.

Please let me know if any further changes or additional information are needed. Thanks for your time and support!

benieric · 2025-01-31T20:12:39Z

Hey sorry for the delay! Thanks for raising this fix and adding tests will look to get this merged ASAP

benieric · 2025-01-31T20:16:09Z

Aiming to get this included in next release if tests are passing, thanks!

benieric · 2025-01-31T20:28:40Z

Can you run tox -e black-format to fix codestyle errors. We'll get this in the next release once tests passing

Julfried · 2025-02-01T10:03:45Z

Hey @benieric,

No worries about the delay—I really appreciate your help! I’ve fixed the formatting style as requested.
Just glad to get this merged—let me know if anything else is needed! 🙂

ejh-alexchatwin · 2025-02-26T14:27:25Z

Hi Friends, if this can't be merged for some reason, is there at least a work around?

edit: the workaround is changing 4ish lines of code in one of the files to match this merge.. surely we can merge this?

Julfried · 2025-02-27T10:35:42Z

I agree that this should be merged. As a temporary workaround you can add this to your requirements.txt file to install the fork where the fix is applied if you are on a windows machine:

git+https://github.com/Julfried/sagemaker-python-sdk.git ; sys_platform == "win32"
sagemaker >=2.228 ; sys_platform != "win32"

@benieric could you please approve so that the tests can run?

ejh-alexchatwin · 2025-02-27T13:31:41Z

Thanks - that's a much tidier workaround than my current hack 😅

benieric · 2025-03-04T00:41:52Z

Seems there are still some codestyles failing if you could take a look

Julfried · 2025-03-04T07:31:59Z

Thanks, I fixed the line too long error, so hopefully it passes now. Unfortunately I am unable to run the tests locally using tox. But when running them with plain pytest they pass.

mollyheamazon · 2025-03-05T20:59:05Z

Could you please take a look at the failed PR checks? Thanks!

* Replace Path.match("/") with Path.anchor comparison * Fix infinite loop in _studio.py path traversal

Julfried · 2025-03-06T08:35:02Z

Rebased to skip the deprecated instance type. Lets see if it works now

Julfried · 2025-03-06T21:15:04Z

I am very sorry for all the failing runs. I fixed the broken test_case for non windows machines and verified that the tests are passing on WSL on my laptop. Hopefully this works now!

ejh-alexchatwin · 2025-03-06T21:30:51Z

Honestly I'm impressed you're still putting in effort to align with their rules given you're working to fix the existing nicely-formatted-but-broken code. thank you!

Julfried · 2025-03-06T21:41:39Z

To be honest, I've already invested so much time and effort in troubleshooting in debugging this frustrating issue. So I thought, why not just push through to get it solved for everyone ;)

mollyheamazon · 2025-03-06T21:52:44Z

@Julfried Thank you for your contribution and your dedication! Let's see if this issue is fixed by now.

Julfried · 2025-03-10T22:32:35Z

Since all tests are passing, I think this can be merged now.

ejh-alexchatwin · 2025-03-11T08:31:10Z

Thanks everyone, especially @Julfried, for resolving this very-hard-to-debug bug!

* fix: resolve Windows path handling in _find_config * Replace Path.match("/") with Path.anchor comparison * Fix infinite loop in _studio.py path traversal * test: Add tests for the new root path exploration * Fix formatting style * Fixed line to long * Fix docstyle by running black manually * Fix testcase with \\ when running on non-windows machines * Fix formatting style * cleanup unused import

achamma723 · 2025-03-27T13:38:13Z

Hello, we have the same problem on Macos. Any ideas for a fix?

* fix: resolve Windows path handling in _find_config * Replace Path.match("/") with Path.anchor comparison * Fix infinite loop in _studio.py path traversal * test: Add tests for the new root path exploration * Fix formatting style * Fixed line to long * Fix docstyle by running black manually * Fix testcase with \\ when running on non-windows machines * Fix formatting style * cleanup unused import

author Molly He <[email protected]> 1740529460 -0800 committer Molly He <[email protected]> 1744156034 -0700 parent fb22b91 author Molly He <[email protected]> 1740529460 -0800 committer Molly He <[email protected]> 1744155953 -0700 parent fb22b91 author Molly He <[email protected]> 1740529460 -0800 committer Molly He <[email protected]> 1744155703 -0700 parent fb22b91 author Molly He <[email protected]> 1740529460 -0800 committer Molly He <[email protected]> 1744155683 -0700 Test py312 ci support Test py312 ci support first commit test to point to personal stack changed tox.ini Revert "changed tox.ini" This reverts commit 696cb47. Revert "Revert "changed tox.ini"" This reverts commit 5d35bd0. Revert "Revert "Revert "changed tox.ini""" This reverts commit 2ab95fb. Revert "Revert "changed tox.ini"" This reverts commit 5d35bd0. Revert "changed tox.ini" This reverts commit 696cb47. Revert "test to point to personal stack" This reverts commit 49de844. Revert "first commit" This reverts commit 1374184. add pyproject.toml add py312 to ci bump numpy version numpy version change add py312 upgrade pip version add setuptools wheel to tox.ini add pyyaml version constraint, remove py312 from docstring because there is no py312 image yet update pyyaml version constraint update pyyaml version constraint deprecate py38 bump scipy bump tensorflow and tensorboard bump dill bump apache-airflow to ensure dill and greenlet version remove constraint for apache-airflow remove constraint for apache-airflow bump torch version bump torchvision version new tests try changing some tests update model trainer test fix test_pipeline add constraint to apache in tox Fix key error in _send_metrics() (aws#5068) Co-authored-by: pintaoz <[email protected]> Use sagemaker session's s3_resource in download_folder (aws#5064) Co-authored-by: pintaoz <[email protected]> Fix error when there is no session to call _create_model_request() (aws#5062) * Fix error when there is no session to call _create_model_request() * Fix codestyle --------- Co-authored-by: pintaoz <[email protected]> Ensure Model.is_repack() returns a boolean (aws#5060) * Ensure Model.is_repack() returns a boolean * update test --------- Co-authored-by: pintaoz <[email protected]> feat: Make DistributedConfig Extensible (aws#5039) * feat: Make DistributedConfig Extensible * pylint * Include none types when creating config jsons for safer reference * fix: update test to account for changes * format * Add integ test * pylint * prepare release v2.240.0 * update development version to v2.240.1.dev0 * Fix key error in _send_metrics() (aws#5068) Co-authored-by: pintaoz <[email protected]> * fix: Added check for the presence of model package group before creating one (aws#5063) Co-authored-by: Keshav Chandak <[email protected]> * Use sagemaker session's s3_resource in download_folder (aws#5064) Co-authored-by: pintaoz <[email protected]> * remove union * fix merge artifact * Change dir path to distributed_drivers * update paths --------- Co-authored-by: ci <ci> Co-authored-by: pintaoz-aws <[email protected]> Co-authored-by: pintaoz <[email protected]> Co-authored-by: Keshav Chandak <[email protected]> Co-authored-by: Keshav Chandak <[email protected]> Skip tests with deprecated instance type (aws#5077) Co-authored-by: pintaoz <[email protected]> pipeline definition function doc update (aws#5074) Co-authored-by: Rohan Gujarathi <[email protected]> feat: add integ tests for training JumpStart models in private hub (aws#5076) * feat: add integ tests for training JumpStart models in private hub * fixed formatting * remove unused imports * fix unused imports * fix unit test failure and fix bug around versioning * fix formatting * fix unit tests * fix model_uri usage issue * fix some formatting * separate private hub setup code * add try catch block * fix flake8 issue so except clause is not bare * black formatting fix: resolve infinite loop in _find_config on Windows systems (aws#4970) * fix: resolve Windows path handling in _find_config * Replace Path.match("/") with Path.anchor comparison * Fix infinite loop in _studio.py path traversal * test: Add tests for the new root path exploration * Fix formatting style * Fixed line to long * Fix docstyle by running black manually * Fix testcase with \\ when running on non-windows machines * Fix formatting style * cleanup unused import change: update image_uri_configs 03-11-2025 07:18:09 PST Fixing Pytorch training python version in tests (aws#5084) * Fixing Pytorch training python version in tests * Updating Inference test handling remove s3 output location requirement from hub class init (aws#5081) * remove s3 output location requirement from hub class init * fix integ test hub * lint * fix test --------- Co-authored-by: Gokul Anantha Narayanan <[email protected]> fix: Prevent RunContext overlap between test_run tests (aws#5083) Co-authored-by: Gokul Anantha Narayanan <[email protected]> remove py38 from unit testing fix integ test by bumping py38 to py39 for PyTorch change framework_version that supports py39 in integ tests remove py38 from unit testing Update estimator.py

* fix: resolve Windows path handling in _find_config * Replace Path.match("/") with Path.anchor comparison * Fix infinite loop in _studio.py path traversal * test: Add tests for the new root path exploration * Fix formatting style * Fixed line to long * Fix docstyle by running black manually * Fix testcase with \\ when running on non-windows machines * Fix formatting style * cleanup unused import

Julfried requested a review from a team as a code owner December 17, 2024 16:08

Julfried requested a review from nargokul December 17, 2024 16:08

Julfried had a problem deploying to manual-approval December 17, 2024 16:08 — with GitHub Actions Failure

Julfried force-pushed the master branch from e6d6db7 to 0cb1caa Compare January 27, 2025 14:51

benieric temporarily deployed to manual-approval January 31, 2025 20:13 — with GitHub Actions Inactive

benieric previously approved these changes Jan 31, 2025

View reviewed changes

Julfried dismissed benieric’s stale review via e2e328b February 1, 2025 09:54

Julfried had a problem deploying to manual-approval February 1, 2025 09:55 — with GitHub Actions Failure

benieric temporarily deployed to manual-approval March 4, 2025 00:31 — with GitHub Actions Inactive

Julfried temporarily deployed to manual-approval March 4, 2025 07:29 — with GitHub Actions Inactive

Julfried temporarily deployed to manual-approval March 5, 2025 08:01 — with GitHub Actions Inactive

Julfried and others added 4 commits March 6, 2025 09:34

fix: resolve Windows path handling in _find_config

8925c70

* Replace Path.match("/") with Path.anchor comparison * Fix infinite loop in _studio.py path traversal

test: Add tests for the new root path exploration

62a489e

Fix formatting style

efbdc13

Fixed line to long

6fc188c

Fix docstyle by running black manually

ebaf507

Julfried force-pushed the master branch from a0efed2 to ebaf507 Compare March 6, 2025 08:34

Julfried temporarily deployed to manual-approval March 6, 2025 08:34 — with GitHub Actions Inactive

Julfried added 2 commits March 6, 2025 22:05

Fix testcase with \\ when running on non-windows machines

9079baa

Fix formatting style

74d82cd

Julfried had a problem deploying to manual-approval March 6, 2025 21:12 — with GitHub Actions Error

cleanup unused import

2495232

Julfried temporarily deployed to manual-approval March 6, 2025 21:12 — with GitHub Actions Inactive

benieric approved these changes Mar 10, 2025

View reviewed changes

benieric merged commit 8a6ab21 into aws:master Mar 10, 2025
14 checks passed

mollyheamazon mentioned this pull request Mar 10, 2025

fit() function hangs indefinitely with no logging or debugging info #4967

Closed

mollyheamazon mentioned this pull request Apr 9, 2025

Py312 upgrade step 2: Update dependencies, integ tests and unit tests #5123

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve infinite loop in _find_config on Windows systems #4970

fix: resolve infinite loop in _find_config on Windows systems #4970

Julfried commented Dec 17, 2024 •

edited

Loading

Julfried commented Dec 21, 2024

ahmednmourad commented Dec 31, 2024

Julfried commented Dec 31, 2024

Julfried commented Jan 16, 2025

Julfried commented Jan 27, 2025

benieric commented Jan 31, 2025

benieric commented Jan 31, 2025 •

edited

Loading

benieric commented Jan 31, 2025

Julfried commented Feb 1, 2025

ejh-alexchatwin commented Feb 26, 2025 •

edited

Loading

Julfried commented Feb 27, 2025 •

edited

Loading

ejh-alexchatwin commented Feb 27, 2025

benieric commented Mar 4, 2025

Julfried commented Mar 4, 2025

mollyheamazon commented Mar 5, 2025

Julfried commented Mar 6, 2025

Julfried commented Mar 6, 2025 •

edited

Loading

ejh-alexchatwin commented Mar 6, 2025

Julfried commented Mar 6, 2025

mollyheamazon commented Mar 6, 2025

Julfried commented Mar 10, 2025 •

edited

Loading

ejh-alexchatwin commented Mar 11, 2025

achamma723 commented Mar 27, 2025 •

edited

Loading

fix: resolve infinite loop in _find_config on Windows systems #4970

fix: resolve infinite loop in _find_config on Windows systems #4970

Conversation

Julfried commented Dec 17, 2024 • edited Loading

Merge Checklist

General

Tests

Julfried commented Dec 21, 2024

ahmednmourad commented Dec 31, 2024

Julfried commented Dec 31, 2024

Julfried commented Jan 16, 2025

Julfried commented Jan 27, 2025

benieric commented Jan 31, 2025

benieric commented Jan 31, 2025 • edited Loading

benieric commented Jan 31, 2025

Julfried commented Feb 1, 2025

ejh-alexchatwin commented Feb 26, 2025 • edited Loading

Julfried commented Feb 27, 2025 • edited Loading

ejh-alexchatwin commented Feb 27, 2025

benieric commented Mar 4, 2025

Julfried commented Mar 4, 2025

mollyheamazon commented Mar 5, 2025

Julfried commented Mar 6, 2025

Julfried commented Mar 6, 2025 • edited Loading

ejh-alexchatwin commented Mar 6, 2025

Julfried commented Mar 6, 2025

mollyheamazon commented Mar 6, 2025

Julfried commented Mar 10, 2025 • edited Loading

ejh-alexchatwin commented Mar 11, 2025

achamma723 commented Mar 27, 2025 • edited Loading

Julfried commented Dec 17, 2024 •

edited

Loading

benieric commented Jan 31, 2025 •

edited

Loading

ejh-alexchatwin commented Feb 26, 2025 •

edited

Loading

Julfried commented Feb 27, 2025 •

edited

Loading

Julfried commented Mar 6, 2025 •

edited

Loading

Julfried commented Mar 10, 2025 •

edited

Loading

achamma723 commented Mar 27, 2025 •

edited

Loading