cleanup: Streamline repository and improve CI/CD setup#2
Conversation
- Remove Docker files (Dockerfile, .dockerignore) - no longer needed - Remove .vscode/ IDE-specific configuration - Remove .pre-commit-hooks.yaml (had linting errors) - Remove GOALS.md (implementation complete) - Remove redundant pull-request.yml workflow (main CI covers PRs)
- Add .vscode/ to .gitignore and update lock file handling - Fix line length in find_experiment_in_project function signature - Add S110 exception for legitimate try-except-pass patterns - Fix test mock expectations for task listing and search - Remove temporary test file causing CI failures
- Update demo video URL to 3eUmgiUJGRVYa9uEJXz5oPXFj - Correct tool count from 12 to 14 tools - Add missing tools to documentation (find_project_by_pattern, find_experiment_in_project) - Simplify requirements section with clearer user requirements - Add uv installation guide link
- Move requirements before Quick Start for better UX - Users now see prerequisites before installation steps
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @prassanna-ravishankar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on enhancing the maintainability and user experience of the repository by performing a comprehensive cleanup, refining dependency management, and updating documentation. It also expands the ClearML Model Context Protocol (MCP) with new capabilities for finding projects and experiments, and improves the robustness of existing task retrieval functions.
Highlights
- Repository Streamlining & Cleanup: Removed various unnecessary files and configurations, including Docker files,
.vscode/IDE settings,.pre-commit-hooks.yaml,GOALS.md, and a redundant CI workflow, to simplify the repository structure. - Dependency Management Enhancement: Updated
.gitignoreto properly handle lock files, specifically trackinguv.lockfor reproducible dependency resolution while ignoringpoetry.lockandPipfile.lock. - Code Quality & Test Fixes: Addressed linting issues across the codebase and tests, resolved test mock expectations for task operations, and removed a temporary test file that was causing CI failures.
- Documentation Improvements: Updated the
README.mdto reflect the latest demo video URL, corrected the reported tool count from 12 to 14, added documentation for newly introduced tools (find_project_by_pattern,find_experiment_in_project), and simplified/repositioned the requirements section for clarity. - New ClearML MCP Tools: Introduced two new ClearML Model Context Protocol (MCP) tools:
find_project_by_patternfor searching projects by name andfind_experiment_in_projectfor locating specific experiments within a given project. - Robust Task Information Retrieval: Refactored
list_tasksandsearch_tasksfunctions inclearml_mcp.pyto first query task IDs and then fetch full task objects, improving robustness by handling potential errors for individual tasks gracefully. - Example Script Refinement: The
examples/consolidated_debugger.pyscript was significantly refactored from a multi-mode debugger into a more focused and streamlined ClearML MCP demo script, showcasing the new project and experiment finding capabilities.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request provides a good cleanup of the repository and improves the developer experience. I've identified a critical security issue with a hardcoded API key that must be addressed. Additionally, I've found several areas where performance can be significantly improved by addressing N+1 query patterns. My other comments focus on improving the robustness of the code. Once these points are addressed, the PR will be in great shape.
| model_id="gemini-2.0-flash", | ||
| api_base="https://generativelanguage.googleapis.com/v1beta/openai/", | ||
| api_key=GEMINI_API_KEY, | ||
| api_key="AIzaSyDAdEToKdFt8SHs25ABz65bx6cedU_zreo", |
There was a problem hiding this comment.
Critical Security Vulnerability: Hardcoded API Key
A hardcoded API key has been committed to the repository. This is a significant security risk, as it exposes credentials to anyone with access to the codebase. Secrets like API keys should never be stored in source code.
Instead, please load the key from an environment variable. This is a standard practice for managing secrets securely.
| api_key="AIzaSyDAdEToKdFt8SHs25ABz65bx6cedU_zreo", | |
| api_key=os.getenv("GEMINI_API_KEY"), |
| for task_id in task_ids: | ||
| try: | ||
| task = Task.get_task(task_id=task_id) | ||
| tasks.append( | ||
| { | ||
| "id": task.id, | ||
| "name": task.name, | ||
| "status": task.status, | ||
| "project": task.get_project_name(), | ||
| "created": str(task.data.created), | ||
| "tags": list(task.data.tags) if task.data.tags else [], | ||
| } | ||
| ) | ||
| except Exception as e: | ||
| # If we can't get a specific task, include the error but continue | ||
| tasks.append({"id": task_id, "error": f"Failed to get task details: {e!s}"}) |
There was a problem hiding this comment.
Performance: N+1 Query Issue
This loop iterates through task_ids and calls Task.get_task(task_id=task_id) for each one. This creates an N+1 query problem: one query to get the list of IDs, and then N queries to get the details for each task. This will be very inefficient and slow if a project contains a large number of tasks.
To fix this, you should use Task.get_tasks(task_ids=...) to fetch all task objects in a single batch API call. This will significantly improve performance.
| for task_id in task_ids: | ||
| try: | ||
| task = Task.get_task(task_id=task_id) | ||
| if pattern_lower in task.name.lower(): | ||
| matching_experiments.append( | ||
| { | ||
| "id": task.id, | ||
| "name": task.name, | ||
| "status": task.status, | ||
| "project": task.get_project_name(), | ||
| "created": str(task.data.created), | ||
| } | ||
| ) | ||
| except Exception: | ||
| # Skip tasks we can't access - could be permissions or API issues | ||
| pass |
There was a problem hiding this comment.
Performance: N+1 Query Issue
Similar to list_tasks, this function suffers from an N+1 query issue by fetching each task individually inside a loop. This can lead to significant performance degradation on projects with many experiments.
Please consider using Task.get_tasks(task_ids=...) to fetch all experiments in a single batch request.
| except Exception: | ||
| # Skip tasks we can't access - could be permissions or API issues | ||
| pass |
There was a problem hiding this comment.
Correctness: Avoid Swallowing Exceptions
Using except Exception: pass is a dangerous practice as it silently swallows all errors. This can hide bugs, make debugging extremely difficult, and lead to unexpected behavior. If a task cannot be accessed due to permissions or other issues, the user of this tool should be notified.
I recommend catching the specific exception if possible, or at least logging the error. A better approach, consistent with other functions in this file like list_tasks, would be to append an error dictionary to the results.
| except Exception: | |
| # Skip tasks we can't access - could be permissions or API issues | |
| pass | |
| except Exception as e: | |
| # Report errors instead of silently passing to aid in debugging. | |
| matching_experiments.append({"id": task_id, "error": f"Failed to get task details: {e!s}"}) |
| for task_id in task_ids: | ||
| try: | ||
| task = Task.get_task(task_id=task_id) | ||
|
|
||
| # Check if the task matches the search query | ||
| task_name = task.name.lower() | ||
| task_comment = getattr(task, "comment", "") or "" | ||
| task_tags = list(task.data.tags) if task.data.tags else [] | ||
|
|
||
| if ( | ||
| query_lower in task_name | ||
| or (task_comment and query_lower in task_comment.lower()) | ||
| or any(query_lower in tag.lower() for tag in task_tags) | ||
| ): | ||
| matching_tasks.append( | ||
| { | ||
| "id": task.id, | ||
| "name": task.name, | ||
| "status": task.status, | ||
| "project": task.get_project_name(), | ||
| "created": str(task.data.created), | ||
| "tags": task_tags, | ||
| "comment": task_comment, | ||
| } | ||
| ) | ||
| except Exception as e: | ||
| # If we can't get a specific task, skip it but log the error | ||
| matching_tasks.append( | ||
| { | ||
| "id": task.id, | ||
| "name": task.name, | ||
| "status": task.status, | ||
| "project": task.project, | ||
| "created": str(task.created), | ||
| "tags": list(task.tags) if task.tags else [], | ||
| "comment": task.comment if hasattr(task, "comment") else None, | ||
| }, | ||
| {"id": task_id, "error": f"Failed to get task details: {e!s}"} | ||
| ) |
There was a problem hiding this comment.
Performance: N+1 Query Issue
This function also has an N+1 query problem, as it fetches each task's details in a loop. For searches that could return many tasks, this will be inefficient.
To improve performance, please use Task.get_tasks(task_ids=...) to fetch task details in a single batch operation.
| if line.strip().startswith("EXPERIMENT_ID:"): | ||
| self.experiment_id = line.split("EXPERIMENT_ID:")[1].strip() |
There was a problem hiding this comment.
Maintainability: Improve robustness of LLM output parsing
The current method of parsing the experiment ID using split() can be brittle. If the EXPERIMENT_ID: string were to appear multiple times in the line for some reason, split() would produce more than two parts, and the logic might not behave as expected.
Using partition() is more robust for this kind of task. It splits the string only at the first occurrence of the separator and always returns a 3-tuple, which makes the code more predictable and resilient to unexpected LLM outputs.
| if line.strip().startswith("EXPERIMENT_ID:"): | |
| self.experiment_id = line.split("EXPERIMENT_ID:")[1].strip() | |
| if line.strip().startswith("EXPERIMENT_ID:"): | |
| self.experiment_id = line.partition("EXPERIMENT_ID:")[2].strip() |
- Add test classes for all major functionality: - Model operations (get_model_info, list_models, get_model_artifacts) - Project search (find_project_by_pattern, find_experiment_in_project) - Project operations (list_projects, get_project_stats) - Task comparison (compare_tasks) - Enhanced task search tests - Exception handling and edge cases - Test coverage improvements: - Cover all 14 MCP tools with comprehensive behavioral tests - Test error handling paths and edge cases - Test individual task retrieval failures in list operations - Test empty data handling in metrics and artifacts - Test missing attributes and None values - Fix test patterns to match actual function implementations: - Use .fn() to access FastMCP wrapped functions - Match actual return field names and structures - Proper mock setup for ClearML API patterns - Achieve 98% test coverage (up from 61%), exceeding 65% CI requirement
- Remove tests/__init__.py (empty file) - Remove src/clearml_mcp/__init__.py (only contained docstring) - Python 3.3+ supports implicit namespace packages - Reduces project clutter without affecting functionality
- Add TRY002 ignore for test files (generic exceptions are fine in tests) - Add INP001 ignore globally (we use implicit namespace packages intentionally) - Fix auto-fixable linting issues (unnecessary else/elif, trailing whitespace) - Format code with ruff format
1 participant
Title
Streamline repository and improve CI/CD setup
Description
This PR streamlines the repository by removing unnecessary files and improving the overall development experience. It also fixes linting issues, updates documentation, and ensures all CI checks pass reliably.
Changes
Repository Cleanup:
Dependency Management:
Code Quality:
Documentation:
Additional Information
uv+ ClearML configSubmission Checklist