file handling #68
Merged
file handling #68
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
akgohain
commented
Oct 19, 2025
akgohain
commented
- revised file upload logic to no longer tack on absolute path; now stores data in temp app storage
- change pytc to package vs. submodule
- file cache clear logic revised
…ackage - Remove git submodule configuration (.gitmodules) - Add setup_pytorch_connectomics.sh script to download at commit 20ccfde (v1.0) - Update Dockerfile to download package instead of copying submodule - Update start.sh to auto-setup pytorch_connectomics if missing - Update README.md with new installation instructions - Add pytorch_connectomics/ to .gitignore to prevent accidental commits
- Fix 22 npm vulnerabilities (1 critical, 7 high, 8 moderate, 6 low) - Updated Electron to latest version (^38.3.0) - Added webpack-dev-server override to fix remaining vulnerabilities - All npm vulnerabilities resolved - Pin Python dependencies to specific versions in requirements.txt - fastapi==0.119.0 - uvicorn==0.38.0 - python-multipart==0.0.20 - neuroglancer==2.38 - imageio==2.37.0 - tensorboard==2.20.0 - tensorboard-data-server==0.7.2 - No Python security vulnerabilities found
Critical Fixes: - Fix API error handling syntax errors in client/src/utils/api.js - Add proper environment variable fallbacks for API configuration - Implement proper async process management in server_pytc/services/model.py - Fix training state management bug in ModelTraining.js - Add proper process tracking and cleanup for subprocess operations - Fix Windows startup script (start.bat) to match Unix functionality - Add psutil dependency for better process management TODO items added throughout codebase for remaining improvements. All critical blocking issues identified in pain-points analysis resolved.
- Add proper exception handling for server_api -> server_pytc communication - Add timeout protection (30s) to prevent hanging requests - Provide clear error messages for ConnectionError and Timeout cases - Return structured error responses with both message and error details - Fixes potential hanging when server_pytc is not running or overloaded All endpoints now handle: - ConnectionError: server_pytc not running - Timeout: server_pytc overloaded/unresponsive - Other exceptions: generic error handling
There was a problem hiding this comment.
Pull Request Overview
This PR refactors file handling to support both local file paths and uploaded file storage in temporary app directories, alongside upgrading PyTorch Connectomics from a submodule to a package dependency.
- File upload logic revised to store uploads in temporary storage instead of using absolute paths
- PyTorch Connectomics changed from git submodule to package installation with automated setup scripts
- Enhanced file cache management with proper cleanup mechanisms
Reviewed Changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/api.py | Added comprehensive test mocking for neuroglancer dependencies and file upload testing |
| start.sh/start.bat | Added automated PyTorch Connectomics setup and improved error handling |
| setup_pytorch_connectomics.sh | New script to download and setup PyTorch Connectomics at specific commit |
| server_pytc/services/model.py | Enhanced process management with proper cleanup and psutil integration |
| server_api/main.py | Refactored neuroglancer endpoint to handle both file uploads and path-based requests |
| client/src/views/ModelTraining.js | Improved training workflow with better validation and error handling |
| client/src/utils/api.js | Enhanced API handling to support file uploads via FormData |
| client/src/contexts/GlobalContext.js | Added file state management with sanitization and cache clearing |
| client/src/components/Dragger.js | Improved file handling with metadata enrichment and object URL management |
| client/src/App.js | Added cache bootstrapping component for initialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| REM Setup pytorch_connectomics if not already present | ||
| if not exist "pytorch_connectomics" ( | ||
| echo Setting up pytorch_connectomics... | ||
| call setup_pytorch_connectomics.sh |
There was a problem hiding this comment.
Calling a shell script (.sh) from a Windows batch file (.bat) will fail. Should use a .bat equivalent or use WSL/Git Bash explicitly.
Suggested change
| call setup_pytorch_connectomics.sh | |
| wsl bash setup_pytorch_connectomics.sh |
Comment on lines
+30
to
+35
| temp_file = tempfile.NamedTemporaryFile( | ||
| delete=False, mode="w", suffix=".yaml" | ||
| ) as temp_file: | ||
| temp_file.write(dict["trainingConfig"]) | ||
| temp_filepath = temp_file.name | ||
| command.extend(["--config-file", str(temp_filepath)]) | ||
|
|
||
| # Execute the command using subprocess.call | ||
| print(command) | ||
| ) | ||
| temp_file.write(dict["trainingConfig"]) | ||
| temp_filepath = temp_file.name | ||
| temp_file.close() |
There was a problem hiding this comment.
Use context manager (with statement) for temporary file handling to ensure proper cleanup even if exceptions occur.
Comment on lines
+116
to
+118
| path.unlink() | ||
| except FileNotFoundError: | ||
| pass |
There was a problem hiding this comment.
Consider using path.unlink(missing_ok=True) instead of catching FileNotFoundError, which is more concise and available in Python 3.8+.
Suggested change
| path.unlink() | |
| except FileNotFoundError: | |
| pass | |
| path.unlink(missing_ok=True) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.