Clean up part 1 - #21
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on repository cleanup by removing legacy/duplicated scripts, experiment artifacts, and older week-based reports, while consolidating documentation into a smaller set of up-to-date guides.
Changes:
- Deleted multiple legacy Python modules/scripts under
src/,scripts/, andexperiments/(including generated output samples and reports). - Reorganized and added consolidated documentation (
docs/GCP.md,docs/FINETUNING.md,docs/WANDB.md,docs/QUICK_REFERENCE.md, etc.). - Updated
README.mdand select docs to reflect the new documentation structure.
Reviewed changes
Copilot reviewed 72 out of 84 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/integration/init.py | Removed integration package re-exports/metadata. |
| src/data/augmentation.py | Removed noise augmentation utility module. |
| src/agent_evaluation/false_positive_detector.py | Removed false-positive evaluation tooling. |
| src/agent_evaluation/agent_evaluator.py | Removed agent correction evaluator implementation. |
| src/agent_evaluation/agent_benchmark.py | Removed agent benchmarking script/module. |
| src/agent_evaluation/ablation_tester.py | Removed ablation testing framework. |
| src/agent_evaluation/init.py | Removed evaluation package re-exports/metadata. |
| scripts/verify_setup.py | Removed setup verification script. |
| scripts/setup_path.sh | Removed shell helper for adding gcloud to PATH. |
| scripts/setup_environment.py | Removed environment setup automation script. |
| scripts/quick_setup.sh | Removed GCP prereq/quick setup helper script. |
| scripts/monitor_gcp_costs.py | Removed GCP cost monitoring script. |
| scripts/deploy_to_gcp.py | Removed evaluation deployment script to GCP VM. |
| scripts/deploy_finetuning_to_gcp.py | Removed fine-tuning deployment orchestration script. |
| scripts/INSTALL_GCLOUD.md | Removed standalone gcloud installation doc. |
| experiments/visualize_evaluation_results.py | Removed evaluation visualization script. |
| experiments/test_api.py | Removed API smoke test script. |
| experiments/test_adaptive_scheduler.py | Removed adaptive scheduler test harness script. |
| experiments/sample_outputs/test_baseline_1.txt | Removed committed sample output artifact. |
| experiments/sample_outputs/evaluate_models_1.txt | Removed committed sample output artifact. |
| experiments/sample_outputs/api_response.txt | Removed committed sample output artifact. |
| experiments/run_hyperparameter_sweep.py | Removed W&B sweep runner script. |
| experiments/run_benchmark.py | Removed benchmark runner script. |
| experiments/run_agent_evaluation.py | Removed agent evaluation entrypoint script. |
| experiments/example_usage.py | Removed example usage script for data management. |
| experiments/evaluation_outputs/evaluation_summary.json | Removed committed evaluation output artifact. |
| experiments/evaluation_outputs/evaluation_report.json | Removed committed evaluation output artifact. |
| experiments/evaluation_outputs/benchmark_report.json | Removed committed benchmark output artifact. |
| experiments/evaluate_models.py | Removed model comparison script. |
| experiments/demo_wandb_tracking.py | Removed W&B tracking demo script. |
| experiments/create_test_evaluation.py | Removed test evaluation dataset creator. |
| experiments/comprehensive_test_suite.py | Removed week-4 comprehensive test suite script. |
| docs/WEEK2_DELIVERABLES_REPORT.md | Removed week-based deliverable report. |
| docs/WEEK1_DELIVERABLES_REPORT.md | Removed week-based deliverable report. |
| docs/WANDB.md | Added consolidated W&B integration guide. |
| docs/SETUP_INSTRUCTIONS.md | Updated resource links to point to new consolidated docs. |
| docs/QUICK_START_DATA_MANAGEMENT.md | Removed redundant quick-start doc. |
| docs/QUICK_REFERENCE.md | Added consolidated command/reference guide. |
| docs/LLM_INTEGRATION.md | Added consolidated LLM integration guide. |
| docs/GCP_SETUP_GUIDE.md | Removed older, longer GCP setup guide. |
| docs/GCP.md | Added consolidated GCP guide. |
| docs/FINETUNING.md | Added consolidated fine-tuning guide. |
| docs/EVALUATION_SUMMARY.md | Updated reference to where the summary lives. |
| docs/DATA_MANAGEMENT_SYSTEM.md | Added quick-start section to main data management doc. |
| docs/CONTROL_PANEL.md | Added consolidated control panel guide. |
| data/metadata/adaptive_stt.code-workspace | Added VS Code workspace file. |
| WEEK4_QUICK_REFERENCE.md | Removed week-4 quick reference doc. |
| WEEK3_QUICK_REFERENCE.md | Removed week-3 quick reference doc. |
| WANDB_SWEEPS_BENEFITS.md | Removed older W&B sweeps benefits doc. |
| WANDB_INTEGRATION_SUMMARY.md | Removed older W&B integration summary doc. |
| README.md | Updated docs list/paths to consolidated docs. |
| QUICK_REFERENCE.md | Removed root-level quick reference in favor of docs/QUICK_REFERENCE.md. |
| LLAMA_INTEGRATION_SUMMARY.md | Removed older LLM integration summary doc. |
| FINETUNING_QUICK_START.md | Removed redundant fine-tuning quick start doc. |
| FINETUNING_GUIDE.md | Removed redundant fine-tuning guide doc. |
| CONTROL_PANEL_GUIDE.md | Removed older control panel guide doc. |
Comments suppressed due to low confidence (1)
README.md:755
README.mdstill documentspython scripts/monitor_gcp_costs.py, butscripts/monitor_gcp_costs.pyis deleted in this PR. Please update the README to reference the new cost-monitoring instructions (or remove the command) to avoid broken setup steps.
python scripts/monitor_gcp_costs.py
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "folders": [ | ||
| { | ||
| "path": "../.." | ||
| } | ||
| ] | ||
| } No newline at end of file |
There was a problem hiding this comment.
Committing editor workspace files under data/metadata/ can be surprising since data/ often represents runtime/generated content. Consider moving this to the repo root (or .vscode/) and/or documenting its presence, to avoid tooling/config mixing with data directories.
|
I can't find the course submission folder in the docs. I can't see it in the changes as well though. Can you help bring it back? |
|
Can you edit the PR description to be more descriptive?
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 81 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Create Utility Functions - Fix test pipeline setup - Bash Bugs found during code review and test pipeline fix - Better Error Handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 82 out of 105 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 102 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 80 out of 102 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif isinstance(obj, np.floating): | ||
| return float(obj) | ||
| elif isinstance(obj, np.bool_): | ||
| return bool(obj) |
There was a problem hiding this comment.
The convert_numpy_types function recursively converts numpy types but doesn't handle numpy arrays. If per_sample_results contains numpy arrays (e.g., from model outputs), they won't be converted and may cause JSON serialization errors. Consider adding a case for np.ndarray that converts to a list: elif isinstance(obj, np.ndarray): return obj.tolist().
| return bool(obj) | |
| return bool(obj) | |
| elif isinstance(obj, np.ndarray): | |
| return [convert_numpy_types(item) for item in obj.tolist()] |
| except HTTPException: | ||
| raise | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=f"Transcription failed: {str(e)}") |
There was a problem hiding this comment.
The error message 'Transcription failed' is generic and may not provide enough context for debugging. Consider including the model name or additional context, e.g., f\"Transcription failed with model {model}: {str(e)}\".
| raise HTTPException(status_code=500, detail=f"Transcription failed: {str(e)}") | |
| raise HTTPException(status_code=500, detail=f"Transcription failed with model {model}: {str(e)}") |
| "input_features": input_features, | ||
| "max_new_tokens": 128, | ||
| "task": "transcribe", # Explicitly set task to avoid forced_decoder_ids deprecation | ||
| "language": None, # None = auto-detect language (set to "en" for English-only transcription) |
There was a problem hiding this comment.
The comment indicates that language=None enables auto-detection, but it's unclear whether this is the desired behavior for all use cases. Consider documenting in a docstring or configuration whether users should set this to 'en' for better performance on English-only audio.
| if hasattr(self, '_available'): | ||
| return self._available | ||
|
|
||
| # Fallback: check if Ollama is available | ||
| if not OLLAMA_AVAILABLE: | ||
| return False |
There was a problem hiding this comment.
The fallback logic in is_available() checking OLLAMA_AVAILABLE is redundant because _available should always be set during __init__. If __init__ completes without setting _available, that indicates a logic error. Consider removing the fallback or documenting why it's necessary.
| 'auto_correction_enabled': enable_auto_correction, | ||
| 'correction_method': correction_method, | ||
| 'llm_available': self.llm_corrector.is_available() if self.llm_corrector else False, | ||
| 'llm_available': self.llm_corrector.is_available() if (self.llm_corrector and hasattr(self.llm_corrector, 'is_available')) else False, |
There was a problem hiding this comment.
The check hasattr(self.llm_corrector, 'is_available') is defensive but suggests uncertainty about the interface contract. If llm_corrector is expected to always have is_available(), this check is unnecessary. Consider either: (1) defining a protocol/interface for correctors, or (2) removing the hasattr check if all correctors are guaranteed to implement is_available().
| 'llm_available': self.llm_corrector.is_available() if (self.llm_corrector and hasattr(self.llm_corrector, 'is_available')) else False, | |
| 'llm_available': self.llm_corrector.is_available() if self.llm_corrector else False, |
- Create Utility Functions - Fix test pipeline setup - Bash Bugs found during code review and test pipeline fix - Better Error Handling
- Create Utility Functions - Fix test pipeline setup - Bash Bugs found during code review and test pipeline fix - Better Error Handling
- Create Utility Functions - Fix test pipeline setup - Bash Bugs found during code review and test pipeline fix - Better Error Handling
- Create Utility Functions - Fix test pipeline setup - Bash Bugs found during code review and test pipeline fix - Better Error Handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 110 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
.github/workflows/tests.yml:1
- The workflow both (a) caches
~/.cache/pipand (b) installs with--no-cache-dirand then purges the pip cache. This makes the cache step ineffective (and the purge step redundant). Consider either removing theactions/cachestep entirely (and keep--no-cache-dir), or removing--no-cache-dir/purge so the cache can actually speed up subsequent runs.
name: Tests
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.info(f"📝 Transcribing with agent. Model: {model} -> Actual: {stt_model.model_name}, Path: {stt_model.model_path}") | ||
| logger.info(f" Agent's baseline_model: {stt_agent.baseline_model.model_name}, Path: {stt_agent.baseline_model.model_path}") | ||
|
|
||
| with tempfile.NamedTemporaryFile(delete=False, suffix=".wav") as tmp: | ||
| content = await file.read() | ||
| tmp.write(content) | ||
| tmp_path = tmp.name | ||
| # Save uploaded file temporarily | ||
| tmp_path = await handle_audio_upload(file) |
There was a problem hiding this comment.
tmp_path is referenced in the finally block but is not shown as being initialized before the try. If an exception occurs before tmp_path is assigned (e.g., inside handle_audio_upload), the finally block can raise UnboundLocalError. Initialize tmp_path = None before entering the try (like transcribe_baseline does) so cleanup is safe.
| - name: Install Python dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| pip install pytest pytest-cov pytest-mock | ||
| # Use CI requirements with CPU-only PyTorch to save space | ||
| pip install --no-cache-dir -r requirements-ci.txt | ||
| pip install --no-cache-dir pytest pytest-cov pytest-mock | ||
|
|
||
| - name: Clean up pip cache | ||
| run: | | ||
| pip cache purge || true |
There was a problem hiding this comment.
The workflow both (a) caches ~/.cache/pip and (b) installs with --no-cache-dir and then purges the pip cache. This makes the cache step ineffective (and the purge step redundant). Consider either removing the actions/cache step entirely (and keep --no-cache-dir), or removing --no-cache-dir/purge so the cache can actually speed up subsequent runs.
| pip cache purge || true | ||
| df -h | ||
|
|
There was a problem hiding this comment.
Since dependencies are installed with --no-cache-dir, pip cache purge provides little benefit and can add time. If disk pressure is the concern, this step can likely be removed; if install speed is the concern, you could instead keep caching and drop --no-cache-dir/purge.
| pip cache purge || true | |
| df -h | |
| df -h | |
| - name: Clean up pip cache | ||
| run: | | ||
| pip cache purge || true | ||
| df -h | ||
|
|
There was a problem hiding this comment.
With --no-cache-dir installs, there is typically little to purge from the pip cache. If the goal is to reduce disk usage, this step may be unnecessary; if the goal is to speed up runs, consider caching instead of purging.
| - name: Clean up pip cache | |
| run: | | |
| pip cache purge || true | |
| df -h | |
| - **[docs/QUICK_START_DATA_MANAGEMENT.md](docs/QUICK_START_DATA_MANAGEMENT.md)** - Quick start for data management | ||
| - **[docs/DATA_MANAGEMENT_SYSTEM.md](docs/DATA_MANAGEMENT_SYSTEM.md)** - Complete data management API | ||
| - **[docs/QUICK_START_DATA_MANAGEMENT.md](docs/QUICK_START_DATA_MANAGEMENT.md)** - Quick start for data management | ||
| - **[docs/GCP_SETUP_GUIDE.md](docs/GCP_SETUP_GUIDE.md)** - GCP setup instructions |
There was a problem hiding this comment.
These links appear to reference files that were removed/moved in this cleanup (e.g., docs/GCP_SETUP_GUIDE.md was replaced by docs/GCP.md, and QUICK_REFERENCE.md was moved under docs/QUICK_REFERENCE.md). Update the README links to match the new consolidated doc locations to avoid broken navigation.
| - **[docs/GCP_SETUP_GUIDE.md](docs/GCP_SETUP_GUIDE.md)** - GCP setup instructions | |
| - **[docs/GCP.md](docs/GCP.md)** - GCP setup instructions |
| - **[docs/UI_TUTORIAL.md](docs/UI_TUTORIAL.md)** - Control Panel UI walkthrough | ||
| - **[docs/CONTROL_PANEL_GUIDE.md](docs/CONTROL_PANEL_GUIDE.md)** - Complete control panel guide | ||
| - **[docs/FINETUNING_QUICK_START.md](docs/FINETUNING_QUICK_START.md)** - Fine-tuning tutorial | ||
| - **[QUICK_REFERENCE.md](QUICK_REFERENCE.md)** - Quick command reference |
There was a problem hiding this comment.
These links appear to reference files that were removed/moved in this cleanup (e.g., docs/GCP_SETUP_GUIDE.md was replaced by docs/GCP.md, and QUICK_REFERENCE.md was moved under docs/QUICK_REFERENCE.md). Update the README links to match the new consolidated doc locations to avoid broken navigation.
| - **[QUICK_REFERENCE.md](QUICK_REFERENCE.md)** - Quick command reference | |
| - **[docs/QUICK_REFERENCE.md](docs/QUICK_REFERENCE.md)** - Quick command reference |
- Create Utility Functions - Fix test pipeline setup - Bash Bugs found during code review and test pipeline fix - Better Error Handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 80 out of 110 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "input_features": input_features, | ||
| "max_new_tokens": 128, | ||
| "task": "transcribe", # Explicitly set task to avoid forced_decoder_ids deprecation | ||
| "language": None, # None = auto-detect language (set to "en" for English-only transcription) |
There was a problem hiding this comment.
The comment states 'None = auto-detect language', but setting language to None might not actually enable auto-detection in the Whisper model. Verify the Whisper API behavior and update the comment if necessary to reflect the actual behavior when language=None.
| "language": None, # None = auto-detect language (set to "en" for English-only transcription) | |
| "language": None, # No explicit language; set to "en" to force English-only transcription or implement detection separately |
| timestamp_str = metric_data.get('timestamp') | ||
| if not timestamp_str: | ||
| logger.warning("Missing timestamp in performance metric, skipping") | ||
| continue | ||
|
|
||
| metric = PerformanceMetrics( | ||
| timestamp=datetime.fromisoformat(timestamp_str), |
There was a problem hiding this comment.
If fromisoformat fails due to an invalid timestamp format, the exception is caught by the generic handler below, but the error message won't clearly indicate the format issue. Consider adding a specific check or better error message for invalid timestamp formats.
| ## 🎥 Tutorials | ||
|
|
||
| ### Complete System Overview Video |
There was a problem hiding this comment.
The 'Tutorials' section appears four times in the README (lines 777, 807, 828, 850) with identical content. Remove the duplicate sections to clean up the documentation.
Repository cleanup: consolidate docs under docs/, remove unused scripts and experiments, and drop orphaned source and test code. Control panel UI is unchanged and working.
docs/: one place for setup, finetuning, GCP, WANDB, control panel, quick reference, and LLM integration.docs/files (e.g. multiple finetuning/WANDB/control panel guides, WEEK1–4 deliverables and quick refs, QUICK_START_DATA_MANAGEMENT, long GCP/FINETUNING_ORCHESTRATION/WANDB_SWEEPS guides).README.mdanddocs/SETUP_INSTRUCTIONS.mdto point to the consolidated docs.deploy_complete_system,deploy_finetuning_to_gcp,deploy_to_gcp,monitor_gcp_costs,quick_setup.sh,setup_environment,setup_path.sh,verify_setup; andscripts/INSTALL_GCLOUD.md(content folded into docs/GCP.md)