Fixed problem with Windows characters. #171
Conversation
| @@ -124,7 +124,7 @@ def main(args, parser, subparser): | |||
| with open(instruct, "w") as filey: | |||
| filey.writelines(config["instructions"]) | |||
|
|
|||
| if not os.path.exists(dest): | |||
| if not os.path.exists(os.path.abspath(dest)): | |||
| os.system("mkdir -p %s" % dest) | |||
There was a problem hiding this comment.
There won't be an issue for the mkdir -p line? (and for other fixes here, for other usage of the variable in the function context?
There was a problem hiding this comment.
I'm just wondering if you've fixed the right area - eg.., install.py has some hard coded paths with a separator, which I don't think would work on windows?
|
I'm sure some of the calls to os.path.exists() aren't susceptible to the problem. However, it doesn't hurt having a abspath() inside just in case, so I replaced it everywhere. Ideally, the bug should be fixed in the os.path library itself... |
|
I only tried this on Linux. The problem we were having was that the function save_data() inside filesystem.py always returned "None" because os.path.exisits(data_base) was False for existing directories, probably due to some extra character. In general, it would be great if the docker builder would abort when things aren't consistent instead of continuing with a warning. |
|
Gotcha. So doing a universal "os.path.abspath" across every path in the project isn't an ideal solution, especially when it's a random set of changes you haven't tested. Let's start with a fix just to the file where the issue is, and then I'd also suggest changing the hard coded paths in the client install.py to be os.path.join instead of forcing to be a slash, and then we can go from there! Also please bump the patch version and add a corresponding note in the CHANGELOG. Thanks! |
|
Unfortunately this wasn't enough to fix the issue. The call to os.path.exists() returns False randomly, with or without using abspath() in it. What fails is this call inside filesystem.py::save_data()
even though data_base contains a good path. It sometimes work, it sometimes doesn't. I couldn't reproduce this inside the container when running Python3 outside the context of Experiment Factory. In this way, os.path.exisits() always return True. :-/ |
|
A couple more things I've unsuccessfully tried: (1) using Pathlib instead of os.path, (2) repeatedly calling os.path.exists() for 20 seconds with 1 second pause in between. I will try upgrading Python (3.8.10) tomorrow... |
|
At the end, the problem was caused by a race condition between /save and /finish in one of the tests. Let's discard this pull-request. |
Description of the Pull Request (PR):
We had an issue with people using Windows entering invisible characters as part of the paths given as parameters when deploying. os.path.exists() assumes the weird characters are part of the path and return False for otherwise existing files and directories. I added a call to os.path.abspath() globally to address this problem. Please consider merging and further using for future os.path.exists()-calls. Thank you.