Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[timeseries] Fix long path name issue on Windows for Chronos #4052

Merged

Conversation

canerturkmen
Copy link
Contributor

Issue #, if available:

Description of changes:

Previously, models saved to long local pathnames caused test failures due to Windows' MAX_PATH. Our path names are now safe for both Windows' os.path.sep and also truncates the names of models loaded locally to increase readability and avoid MAX_PATH issues.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@canerturkmen canerturkmen added bug Something isn't working OS: Windows Impacting Windows OS module: timeseries related to the timeseries module labels Apr 6, 2024
@canerturkmen canerturkmen added this to the 1.1 Release milestone Apr 6, 2024
@yinweisu
Copy link
Collaborator

yinweisu commented Apr 6, 2024

Previous CI Run Current CI Run

Copy link
Contributor

@AnirudhDagar AnirudhDagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks LGTM, should fix the issue as discussed offline. Left a minor comment.

@AnirudhDagar
Copy link
Contributor

AnirudhDagar commented Apr 6, 2024

/platform_tests ref=fix-chronos-win-path-issue

Platform Tests Output

@canerturkmen
Copy link
Contributor Author

/platform_tests ref=fix-chronos-win-path-issue

This branch is on my fork. Will it still work?

@AnirudhDagar
Copy link
Contributor

AnirudhDagar commented Apr 6, 2024

yes the branch being on fork (i'm not sure if it matters at all if one has the PAT setup, which for some reason didn't work for me) doesn't matter when initiated manually through actions: https://github.com/autogluon/autogluon/actions/runs/8579909126 (now passing for windows, except for 3.8 which should be fixed when we merge #4051)

@canerturkmen canerturkmen merged commit c132f76 into autogluon:master Apr 6, 2024
27 checks passed
@canerturkmen canerturkmen deleted the fix-chronos-win-path-issue branch April 6, 2024 09:19
Copy link

github-actions bot commented Apr 6, 2024

Job PR-4052-531072b is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-4052/531072b/index.html

LennartPurucker pushed a commit to LennartPurucker/autogluon that referenced this pull request Jun 1, 2024
…on#4052)

Co-authored-by: Anirudh Dagar <anirudhdagar6@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module: timeseries related to the timeseries module OS: Windows Impacting Windows OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants