-
Notifications
You must be signed in to change notification settings - Fork 10
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
update: upload_widget #75
Conversation
- Allow for file_types to be a list of str if user wants to upload more than one file type with the same upload widget - Allow for uploading a directory, i.e. uploading a bruker .d file - Allow for local symlink creation instead of copying/duplicating files
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Reviewer Guide 🔍
|
PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Code Suggestions ✨
|
Great idea. An optional symlink feature was in my internal wishlist ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for contributing.
I am also strongly in favour of allowing the use of Symlinks for local deployments as this potentially significantly reduces the amount of storage needed.
However, there are also advantages to separating the WebApp file system from the "real" file system. Most notably, this ensures that files are not altered, renamed, or moved during their life cycle within the WebApp (which can be quite long depending on the WebApp). This definitely does not cancel out the advantage of more efficient use of storage but should be accounted for by developers and thus results in higher complexity. Hence, I would argue to not enable it by default.
Unfortunately, the symlink approach does not work on unaltered windows. Maybe we could instead store the path to the file? This should work on all platforms.
Apply suggested change, change symlink default to False. Co-authored-by: Tom David Müller <57191390+t0mdavid-m@users.noreply.github.com>
Co-authored-by: Tom David Müller <57191390+t0mdavid-m@users.noreply.github.com>
It's a good point for keeping WebApp's separate from the real working directory to avoid unwanted actions being performed on the actual file system. Storing the file path would probably be the best approach for all OS's. Should we add another advanced flag, or use the symlink flag to determine whether the real local file system files should be used? |
Use os.link to create a link to a file in windows or use mklink to link a directory. Not sure if this actually works yet, need to check on a win machine
- fix: if not using symlink and copying directory, use copytree and allow for existing directory
I think the symlink flag should be sufficient. Developers that use that flag will have to use caution and check if the file still exists. We could also check for that in |
Thanks for the productive discussion. You are right, I totally missed that in my original review:
Using |
- added a tkinter based directory selector - removed symlink/hardlink option
Add suggestion to make tk dilog on the top Co-authored-by: Tom David Müller <57191390+t0mdavid-m@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for making the changes! I think this PR is almost good to go. I just noticed a few minor details when checking it out:
- The "Symlink Checkbox" should also be available for single file uploads. We could just replace the upload widget with a big button prompting the TKinter file selection if the "Symlink Checkbox" is unchecked.
- The Current MS data files list in the upload section should include "symlinked" files
- The "Copy MS data files from folder button" should be changed to "Add MS data files from folder button" or similar
- Only files that still exist should be made available as input (see code suggestion)
Co-authored-by: Tom David Müller <57191390+t0mdavid-m@users.noreply.github.com>
Done. I added a big button that calls a tkinter file selection (allows multiple file selection) if the "Symlink Checkbox" is unchecked.
Done. Not sure which way is the best to display, but for now I made it to display as i.e. the alternative would be to just display the whole full filepath name, though it may look messier? i.e. or we could just display the filename and not indicate whether it's local or not 🤷♂️
Done.
Done. |
Thanks for making the changes! I like the version were files are displayed with the (local) tag. I changed one info-box which mistakenly told the user that their files were copied and made some minor adjustments to satisfy the linter. From my side the PR would be ready to merge! |
The failing CI pipeline will be addressed in a separate PR and is unrelated to the changes in this PR. |
User description
I have been playing around with the streamlit-template to create a web-app for Sage. There were a few things I wanted to change with the file upload widget, more so specifically when running a web-app with local. If running a web-app with local, when uploading files, I personally prefer creating symbolic links to the original files (if the purpose is to just read data), to avoid making multiple copies. This I think is probably useful if there are a lot of files or large files that you want to process. I added an option to the file upload widget to create symlinks if requested (default) instead, otherwise make copies when running an app locally.
Let me know what you think.
PR Type
Enhancement, Bug fix
Description
file_type
tofile_types
.Changes walkthrough 📝
Workflow.py
Support multiple file types in upload widget
src/Workflow.py
file_type
tofile_types
to support multiple file types.StreamlitUI.py
Enhance upload widget with multiple file types and symlink option
src/workflow/StreamlitUI.py