Configure size upload limit #156
Conversation
WalkthroughThe changes update the execution flow of the Streamlit application in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as run_app.py
participant Subprocess
participant Streamlit as Streamlit App
User->>Script: Run script with arguments
Script->>Script: Build command using subprocess
Script->>Subprocess: Execute the command
Subprocess->>Streamlit: Start Streamlit app
Streamlit-->>Subprocess: Return execution status
Subprocess-->>Script: Return exit status
alt Error Occurred
Script->>Logger: Log error details
Script->>User: Exit with status code 1
else Execution Successful
Script->>User: Streamlit app is running
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
settings.json (2)
5-5: Consider adding unit specification to the configuration name.The setting uses 200 as the value, but does not indicate the unit of measurement (MB) in either the parameter name or a comment. This might cause confusion for users configuring this value.
- "maximum_file_upload_size": 200, + "maximum_file_upload_size_mb": 200,Alternatively, you could keep the same parameter name but add a comment indicating the unit:
- "maximum_file_upload_size": 200, + "maximum_file_upload_size": 200, /* Size in MB */
5-5: Consider using consistent naming convention for JSON keys.Other configuration keys in this file use kebab-case (e.g., "app-name") while this new setting uses snake_case. For consistency, consider using the same naming convention throughout the file.
- "maximum_file_upload_size": 200, + "maximum-file-upload-size": 200,run_app.py (3)
8-14: Consider improving error handling and path resolution for settings.json.The function correctly handles file-not-found and JSON decode errors, but uses a relative path that assumes the script is run from the repository root. Consider using a more robust path resolution approach.
def get_upload_limit(): """Read max upload size from settings.json or use default.""" try: - with open("settings.json") as f: + # Ensure we look for settings.json relative to this script's location + import os + settings_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "settings.json") + with open(settings_path) as f: return json.load(f).get("maximum_file_upload_size", DEFAULT_MAX_SIZE) except (FileNotFoundError, json.JSONDecodeError): + print(f"Warning: Could not read settings.json. Using default upload size of {DEFAULT_MAX_SIZE}MB.") return DEFAULT_MAX_SIZE
18-20: Enhance the command-line argument description to include units.The help text for the
--max-uploadargument should indicate that the value is in megabytes.- parser.add_argument('--max-upload', type=int, help='Override upload size (MB)') + parser.add_argument('--max-upload', type=int, help='Override maximum file upload size in MB')
26-29: Consider handling subprocess errors gracefully.The current implementation doesn't handle potential errors from the subprocess execution. Add error handling to provide better feedback if Streamlit fails to start.
# Run Streamlit with the configured upload size -subprocess.run([ - "streamlit", "run", "app.py", - "--server.maxUploadSize", str(size) -] + unknown) # Pass through additional arguments +try: + print(f"Starting Streamlit with maximum upload size: {size}MB") + result = subprocess.run([ + "streamlit", "run", "app.py", + "--server.maxUploadSize", str(size) + ] + unknown, check=True) # Pass through additional arguments +except subprocess.CalledProcessError as e: + print(f"Error starting Streamlit: {e}") + exit(1) +except FileNotFoundError: + print("Error: Streamlit command not found. Please ensure Streamlit is installed.") + exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
run_app.py(1 hunks)settings.json(1 hunks)
🔇 Additional comments (1)
run_app.py (1)
1-6: Good job adding a clear constant for the default upload size.Defining the default upload size as a named constant with a comment explaining the units makes the code more maintainable and easier to understand.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
run_app.py (1)
12-18: Consider enhancing error handling in theget_upload_limitfunction.The function correctly reads from settings.json and handles file not found and JSON parsing errors. However, it silently falls back to the default value without logging the specific error.
Consider enhancing the error handling to log specific errors:
def get_upload_limit(): """Read max upload size from settings.json or use default.""" try: with open("settings.json") as f: return json.load(f).get("maximum_file_upload_size", DEFAULT_MAX_SIZE) - except (FileNotFoundError, json.JSONDecodeError): + except FileNotFoundError: + logging.warning("settings.json not found. Using default upload size of %dMB.", DEFAULT_MAX_SIZE) + return DEFAULT_MAX_SIZE + except json.JSONDecodeError: + logging.warning("Invalid JSON in settings.json. Using default upload size of %dMB.", DEFAULT_MAX_SIZE) return DEFAULT_MAX_SIZE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
run_app.py(1 hunks)
🔇 Additional comments (5)
run_app.py (5)
1-11: Well-structured setup with necessary imports and default configuration.The code effectively imports all required modules and establishes a default upload size constant. The logging configuration is appropriately set at the WARNING level, which is reasonable for a production application.
21-24: Good implementation of command-line argument parsing.The code correctly uses argparse to handle the --max-upload parameter and keeps track of unknown arguments to pass through to Streamlit. This approach balances flexibility with clarity.
26-27: Clear priority logic for determining upload size.The code establishes a well-defined priority order for the upload size: command-line argument > settings.json > default value. This approach respects user preferences while providing sensible fallbacks.
29-32: Proper validation for upload size as previously requested.This validation logic ensures the upload size is always positive, addressing the feedback from the previous review. The warning message clearly informs users when the default is being used due to an invalid value.
34-38: Effective implementation of Streamlit execution with configured parameters.The subprocess.run approach correctly passes the upload size to Streamlit while preserving any additional command-line arguments. This implementation allows for both the configured upload size and any other Streamlit-specific parameters to be used.
The additional arguments forwarding is particularly valuable as it maintains compatibility with existing workflows that might use other Streamlit parameters.
There was a problem hiding this comment.
Great start! Please see comments. Nothing wrong with the implementation but I prefer not to introduce argparse for just one argument. @t0mdavid-m might have a different opinion on this.
|
@jcharkow, Thank you. |
|
Sorry I was not aware that streamlit had a native way to change the file upload size. This would be the preferred approach so thank you for updating that. For our sever, we do want to keep the default at 200Mb so please change the value to that. If you could also update the documentation as well so that new users of aware of how to configure this when running locally (e.g. here to say something like "In local mode file size upload limit can be adjusted through the streamlit's .streamlit/config.toml file. with the maxUploadSize parameter" I am still confused about the other changes to run_app.py. Are these necessary because streamlit should have its own logging mechanisms? |
@jcharkow |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/user_guide.md (1)
21-35: Clarify Default Values and Override Examples for Local ModeThe instructions for adjusting the file upload limit in local mode are detailed and informative. However, the wording might confuse users—while it states that the default is 200MB, the example demonstrates setting the limit to 500MB. Consider explicitly clarifying that 200MB is the default and that the examples illustrate how to override this default setting.
Below is a suggested diff to improve clarity:
- - You can modify the `.streamlit/config.toml` file and set the `maxUploadSize` parameter to your desired value. By default, this is set to 200MB. + - You can modify the `.streamlit/config.toml` file and set the `maxUploadSize` parameter (in MB) to your desired value. By default, it is configured to 200MB; the example below shows how to override this default by setting it to 500MB.- - You can customize the file size upload limit directly when running the application using the `--server.maxUploadSize` argument. + - You can customize the file size upload limit directly when running the application using the `--server.maxUploadSize` argument (value in MB). For example, running:- python run_app.py --server.maxUploadSize 500 + python run_app.py --server.maxUploadSize 500This clarification will ensure users understand that 500MB is just an example override and that the system defaults to 200MB if no override is provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/user_guide.md(1 hunks)
🔇 Additional comments (1)
docs/user_guide.md (1)
18-20: Clear Section Header and Online Mode DescriptionThe new "File Uploads" section is clearly marked and the instructions for online mode are concise. This structure helps users quickly differentiate between online and local behaviors for file uploads.
feat: Add configurable file upload size via settings.json and CLI
maximum_file_upload_sizeparameter to settings.jsonResolves #155
@jcharkow Please review the PR and let me know if any changes are needed.
Summary by CodeRabbit
Refactor
New Features