-
Notifications
You must be signed in to change notification settings - Fork 34
Add datasets and models to the upgrade script #288
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
base: release-1.4.0
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This pull request implements comprehensive upgrade functionality for Intel® SceneScape, extending beyond database upgrades to include model and dataset management during the upgrade process.
Key changes:
- Expanded upgrade script to handle models and datasets in addition to database migration
- Renamed upgrade tool from
upgrade-databasetoupgrade-scenescapeto reflect broader scope - Updated documentation and build scripts to reference the new upgrade process
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| percebro/src/open_pose.py | Complete file removal - OpenPose model implementation deleted |
| manager/tools/upgrade-scenescape | Added functions to copy models and datasets to Docker volumes during upgrade |
| docs/user-guide/How-to-upgrade.md | Updated documentation to reference new upgrade script and added volume verification steps |
| deploy.sh | Changed script references from upgrade-database to upgrade-scenescape |
| Makefile | Renamed targets and updated help text to reflect broader upgrade scope |
| local models_src="model_installer/models" | ||
|
|
||
| log_message "Initializing models volume..." | ||
| docker volume rm scenescape_vol-models 2>/dev/null || true |
Copilot
AI
Aug 8, 2025
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.
The script forcibly removes existing volumes without checking if they contain important data. Consider adding a backup or confirmation step before removing volumes to prevent accidental data loss.
| docker volume rm scenescape_vol-models 2>/dev/null || true | |
| if docker volume inspect scenescape_vol-models >/dev/null 2>&1; then | |
| echo "WARNING: This will remove the existing 'scenescape_vol-models' volume and all its data." | |
| read -p "Are you sure you want to continue? [y/N]: " confirm | |
| if [[ "$confirm" != "y" && "$confirm" != "Y" ]]; then | |
| echo "Aborting models volume initialization." | |
| return 1 | |
| fi | |
| docker volume rm scenescape_vol-models | |
| fi |
|
|
||
| log_message "Initializing datasets volume..." |
Copilot
AI
Aug 8, 2025
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.
Similar to the models volume, the datasets volume is removed without verification. This could lead to data loss if the volume contains user data that should be preserved during upgrade.
| log_message "Initializing datasets volume..." | |
| local backup_dir="backup_datasets_$(date +%Y%m%d_%H%M%S)" | |
| log_message "Checking for existing data in scenescape_vol-datasets volume..." | |
| # Use a temporary container to check if the volume is non-empty | |
| docker run --rm \ | |
| -v scenescape_vol-datasets:/data \ | |
| alpine:latest /bin/sh -c "ls -A /data" > /tmp/datasets_volume_contents.txt 2>/dev/null || true | |
| if [ -s /tmp/datasets_volume_contents.txt ]; then | |
| echo "WARNING: Existing data found in scenescape_vol-datasets volume:" | |
| cat /tmp/datasets_volume_contents.txt | |
| echo "Do you want to back up the existing datasets before removal? [y/N]" | |
| read -r backup_choice | |
| if [ "$backup_choice" = "y" ] || [ "$backup_choice" = "Y" ]; then | |
| mkdir -p "$backup_dir" | |
| docker run --rm \ | |
| -v scenescape_vol-datasets:/data \ | |
| -v ${PWD}/$backup_dir:/backup \ | |
| alpine:latest /bin/sh -c "cp -r /data/* /backup/" | |
| echo "Backup completed: $backup_dir" | |
| fi | |
| echo "Do you want to delete the existing datasets volume? This will permanently remove all data. [y/N]" | |
| read -r delete_choice | |
| if [ ! "$delete_choice" = "y" ] && [ ! "$delete_choice" = "Y" ]; then | |
| echo "Aborting datasets volume removal." | |
| return 1 | |
| fi | |
| fi | |
| rm -f /tmp/datasets_volume_contents.txt | |
| log_message "Removing and re-creating datasets volume..." |
| docker volume rm scenescape_vol-models 2>/dev/null || true | ||
| docker volume create scenescape_vol-models | ||
|
|
||
| docker run --name ${temp_container} \ |
Copilot
AI
Aug 8, 2025
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.
The temporary container creation doesn't handle the case where a container with the same name already exists. Consider adding docker rm -f ${temp_container} 2>/dev/null || true before creating the container to prevent conflicts.
| log_message "Initializing datasets volume..." | ||
| docker volume rm scenescape_vol-datasets 2>/dev/null || true | ||
| docker volume create scenescape_vol-datasets | ||
|
|
Copilot
AI
Aug 8, 2025
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.
Same issue as with the models container - potential naming conflicts if a container with the same name already exists from a previous failed run.
| # Remove any existing container with the same name to avoid naming conflicts | |
| docker rm -f ${temp_container} 2>/dev/null || true |
| alpine:latest sleep 10 | ||
|
|
||
| log_message "Copying models to volume..." | ||
| docker exec ${temp_container} /bin/sh -c "cp -r /source/models/* /dest/models/" |
Copilot
AI
Aug 8, 2025
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.
The copy command will fail if the source directory /source/models/ is empty or doesn't contain any files, as the shell expansion /* will not match anything. Consider using cp -r /source/models/. /dest/models/ or adding a check for directory contents first.
| docker exec ${temp_container} /bin/sh -c "cp -r /source/models/* /dest/models/" | |
| docker exec ${temp_container} /bin/sh -c "cp -r /source/models/. /dest/models/" |
| alpine:latest sleep 10 | ||
|
|
||
| log_message "Copying datasets to volume..." | ||
| docker exec ${temp_container} /bin/sh -c "cp -r /source/datasets/* /dest/datasets/" |
Copilot
AI
Aug 8, 2025
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.
Same issue as with the models copy - the command will fail if /source/datasets/ is empty. The shell glob /* expansion requires at least one file to match.
| docker exec ${temp_container} /bin/sh -c "cp -r /source/datasets/* /dest/datasets/" | |
| docker exec ${temp_container} /bin/sh -c "if [ \"\$(ls -A /source/datasets)\" ]; then cp -r /source/datasets/* /dest/datasets/; else echo 'No datasets to copy.'; fi" |
📝 Description
Provide a clear summary of the changes and the context behind them. Describe what was changed, why it was needed, and how the changes address the issue or add value.
✨ Type of Change
Select the type of change your PR introduces:
🧪 Testing Scenarios
Describe how the changes were tested and how reviewers can test them too:
✅ Checklist
Before submitting the PR, ensure the following: