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

Fix this line with File.separator in the Save and Load preprocessors #226

Closed
zaleslaw opened this issue Sep 20, 2021 · 4 comments · Fixed by #234
Closed

Fix this line with File.separator in the Save and Load preprocessors #226

zaleslaw opened this issue Sep 20, 2021 · 4 comments · Fixed by #234
Assignees
Labels
bug Something isn't working
Milestone

Comments

@zaleslaw
Copy link
Collaborator

How about fix this line with File.separator? This is not correct for unix systems now.

Originally posted by @YokiToki in #209 (comment)

@zaleslaw zaleslaw added the bug Something isn't working label Sep 20, 2021
@zaleslaw zaleslaw added this to the 0.3 milestone Sep 20, 2021
@zaleslaw zaleslaw self-assigned this Sep 20, 2021
@juliabeliaeva
Copy link
Contributor

There are several places where windows separator are used. Another one is here:
https://github.com/JetBrains/KotlinDL/blob/540bf0316d38d6216ef99b8afd8403cdd72e8889/api/src/main/kotlin/org/jetbrains/kotlinx/dl/dataset/handler/Cifar10Util.kt#L36
I have the patch for these two places, but as it conflicts with changes in #225, I have not send it yet.

There are also usages of windows separators and absolute paths in several jupyter files, those should be fixed as well, maybe in another issue.

@zaleslaw
Copy link
Collaborator Author

zaleslaw commented Sep 21, 2021

@juliabeliaeva I merged the #225 .
It will be really helpful to get this patch or PR, but I'm a little bit confused about this separator (I've tested the 0.1 and 0.2 release on Ubuntu and macOS and it seems to work fine on both of them with the same separator).

But I don't test all examples and tests and for example, Cifar10 too. On another side, all examples run a lot of times on TC on Ubuntu and macOS successfully.

Looks like I need to include in the required pre-release testing procedure manually running of all examples on different TC

P.S. I remember this problem with separators a few years ago when I was working on a project which used a file system a lot of the time and it supports multiple filesystems, so I used different separators for each of them.

P.P.S I recognized that Cifar10Util contains unused code (outdated special loaders for Cifar10 dataset), this is a reason why it's not raised during TC runs (with high level probability I'll remove it in two days)

@juliabeliaeva
Copy link
Contributor

Save preprocessor works with windows separators, it just saves the file in the wrong place and I guess this is not tested. As for Cifar10Util, yes, the code is unused.

@zaleslaw
Copy link
Collaborator Author

Agree, I never tested Save preprocessor yet. So, thanks for fix, I've run TC for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants