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

On Windows, prevent long trial directory names #735

Merged
merged 4 commits into from
Mar 8, 2024

Conversation

tg2k
Copy link
Contributor

@tg2k tg2k commented Aug 28, 2023

On Windows, prevent long trial directory names (ray-project/ray#29586, #526).

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CLAassistant
Copy link

CLAassistant commented Aug 28, 2023

CLA assistant check
All committers have signed the CLA.

@jmoralez jmoralez requested a review from cchallu September 13, 2023 15:41
@jmoralez
Copy link
Member

jmoralez commented Nov 6, 2023

Hey @tg2k, are you still interested in moving this forward? The argument should either be a function or None, right now it's always a function and it can return None, which is what is causing the errors.

@tg2k
Copy link
Contributor Author

tg2k commented Nov 6, 2023

Thanks @jmoralez, I hadn't realized. I pushed another change that may help.

@jmoralez
Copy link
Member

Hey @tg2k. Are you able to provide a reproducible example of this? I've been trying to do that in #814 but it passes haha. If you can provide one I can include it there and merge this to make sure it fixes the issue. Then we can merge this to main.

@tg2k
Copy link
Contributor Author

tg2k commented Nov 29, 2023

@jmoralez It's been about 3 months since I made a stab at a Windows environment, so my memory in this is a little fuzzy, and I don't have a reproducible example.

I believe my Windows-side directory path was 21 characters long. As an example of a Windows path that could be created, from #526 (comment) you can see a path like

C:\pathto\ray_results\train_tune_2023-04-20_15-07-40\train_tune_7f3f4_00000_0_check_val_every_n_epoch=100,hidden_size=512,hist_exog_list=spx_log_ret,input_size=20,learning_rate=0.0012_2023-04-20_15-07-43\lightning_logs\version_0\events.out.tfevents.1681974471.Desktop-DC.30528.0

which is about 279 characters long. Windows maxes out at 260 characters unless you enable extended-length paths (as shown on that page), but for the most part support for those long paths is weak in Windows.

You can see from the example above that the default behavior encodes a lot of parameters into the path. If you have a long enough base path, or enough parameters (such as exogenous variables), you'll push the path over the limit (unless Ray-Tune has changed how it creates paths on Windows in the meantime).

@elephaint
Copy link
Contributor

elephaint commented Mar 8, 2024

I can reproduce too long filenames on Windows 11, but it doesn't error on my machine - Ray gives a warning that the filename exceeds 260 characters. The code to give Ray >260 character file dir names is a bit hacky, @jose-moralez do you want me to give an example for that? It's basically the below code, inserted at this point. This ensures that the file directory of the notebook/file that is ran is used to write Ray's results rather than the default Ray results directory, resulting in excessive file dir path lengths:

        from pathlib import Path
        current_dir = Path(__file__).parent
        result_dir = current_dir.joinpath("results/")

        tuner = tune.Tuner(
            tune.with_resources(train_fn_with_parameters, device_dict),
            run_config=air.RunConfig(callbacks=self.callbacks, verbose=verbose, local_dir=result_dir),
            tune_config=tune.TuneConfig(
                metric="loss",
                mode="min",
                num_samples=num_samples, 
                search_alg=search_alg,
            ),
            param_space=config,
        )

Subsequently, I can confirm the fix proposed in this PR does reduce the file dir name and shows that the file dir name will be correctly reduced on my machine. Ray also no longer provides a warning that the file dir name is too long. So it does 'fix' the long file dir name, but I could not reproduce the error before applying the fix, although I could (with the code above) reproduce the 'too long file dir name' on Windows with Ray.

A potential issue with the fix of this PR is that trial_dirname_creator is yet an alpha feature

I'd propose to apply the PR, it seems to fix the issue.

@jmoralez jmoralez merged commit 906ea9f into Nixtla:main Mar 8, 2024
15 checks passed
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.

4 participants