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

Lightning Storage doesn't support filenames with spaces #15271

Open
yurijmikhalevich opened this issue Oct 24, 2022 · 4 comments
Open

Lightning Storage doesn't support filenames with spaces #15271

yurijmikhalevich opened this issue Oct 24, 2022 · 4 comments
Labels
app Generic label for Lightning App package bug Something isn't working

Comments

@yurijmikhalevich
Copy link
Member

yurijmikhalevich commented Oct 24, 2022

Bug description

Lightning Storage doesn't support filenames with spaces; it crashes with an access error if the user tries to upload a file that contains a space. We need to either add the support or make sure that:

  1. Lightning Storage docs mention this as a limitation.
  2. Framework prints a meaningful error if the user tries to upload a file with spaces in it.

I vote for adding the support of any valid filenames to the Lightning Storage:

In my eyes, Lightning Storage API is intended to be used alongside fs writes and reads primarily to store the app artifacts consistently between app restarts and recreations (and even share it between apps). Why would Lightning Storage API care about spaces in this use case? These files aren't being exposed as links anywhere. And, if the Lightning App developer decides to expose these files as links, they probably should know to avoid spaces, and if they choose not to, it's OK to support them still, IMO.

Of course, we should be converting spaces to-and-from %20 in the framework, and in the web app Artifacts UI, the user should see spaces, not %20, everywhere.

My vote is for adding the whitespace support because it streamlines the development experience instead of hindering it by showing an error.

How to reproduce the bug

Try to upload a file with a space in it to the Lightning Drive.

Error messages and logs

No response

Environment


#- Lightning Component (e.g. Trainer, LightningModule, LightningApp, LightningWork, LightningFlow): N/A
#- PyTorch Lightning Version (e.g., 1.5.0): N/A
#- Lightning App Version (e.g., 0.5.2): 0.7.0
#- PyTorch Version (e.g., 1.10): N/A
#- Python version (e.g., 3.9): cloud
#- OS (e.g., Linux): Linux
#- CUDA/cuDNN version: N/A
#- GPU models and configuration: N/A
#- How you installed Lightning(`conda`, `pip`, source): `pip`
#- Running environment of LightningApp (e.g. local, cloud): cloud

More info

No response

@yurijmikhalevich yurijmikhalevich added the needs triage Waiting to be triaged by maintainers label Oct 24, 2022
@awaelchli
Copy link
Member

awaelchli commented Oct 24, 2022

I vote for error message. In the cloud, it is not so uncommon to encounter file names in links. It can be inconvenient to work with them if the spaces are encoded as %20.

@awaelchli awaelchli added app Generic label for Lightning App package bug Something isn't working and removed needs triage Waiting to be triaged by maintainers labels Oct 24, 2022
@yurijmikhalevich
Copy link
Member Author

yurijmikhalevich commented Oct 24, 2022

@awaelchli, hi! 👋

It can be inconvenient to work with them if the spaces are encoded as %20.

But should this be a decision enforced by Lightning Storage or something we leave for the Lightning App developers to decide?

In my eyes, Lightning Storage API is intended to be used alongside fs writes and reads primarily to store the app artifacts consistently between app restarts and recreations (and even share it between apps). Why would Lightning Storage API care about spaces in this use case? These files aren't being exposed as links anywhere. And, if the Lightning App developer decides to expose these files as links, they probably should know to avoid spaces, and if they choose not to, it's OK to support them still, IMO.

Of course, we should be converting spaces to-and-from %20 in the framework, and in the web app Artifacts UI, the user should see spaces, not %20, everywhere.

My vote is for adding the whitespace support because it streamlines the development experience instead of hindering it by showing an error.

@awaelchli
Copy link
Member

That makes sense. The storage call could occur anytime in an app after running a long time so an error is not useful. Also because the developer of the app might not wand to handle this themselves for their users.

Feel free to also include such concrete proposals and considerations/learnings into the issue description directly next time.

@stale
Copy link

stale bot commented Apr 14, 2023

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions - the Lightning Team!

@stale stale bot added the won't fix This will not be worked on label Apr 14, 2023
@yurijmikhalevich yurijmikhalevich removed the won't fix This will not be worked on label Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Generic label for Lightning App package bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants