WIP: feat(multithreading): imprement support for multithreading and fix linter warning #87
Conversation
- Implement concurrent commit generation using ThreadPoolExecutor - Add configurable thread count via --max_threads flag (default: 4) - Integrate tqdm progress bar with graceful fallback - Add thread-safe git operations with locking mechanism - Include requirements.txt for dependency management
Add type ignore comment on line 80 where tqdm is called. The warning is a false positive since tqdm is only used when TQDM_AVAILABLE is True, meaning the import succeeded. The type checker cannot infer this runtime condition, so the ignore directive is necessary.
There was a problem hiding this comment.
Pull request overview
This PR adds multithreading support to the GitHub Activity Generator script to improve performance, along with an optional progress bar feature using tqdm for better user experience during commit generation.
- Implements parallel commit generation using
ThreadPoolExecutorwith configurable thread count - Adds progress tracking with tqdm library (optional) and a fallback simple progress indicator
- Updates documentation with multithreading examples, performance benchmarks, and installation instructions
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| contribute.py | Adds multithreading with ThreadPoolExecutor, progress bar integration, thread-safe git operations via global lock, and new --max_threads CLI argument |
| test_contribute.py | Reformats test code to follow modern Python style conventions (double quotes, better line breaks) |
| requirements.txt | Adds tqdm dependency with minimum version 4.65.0 |
| .github/README.md | Documents multithreading feature, progress bar usage, performance improvements, and installation instructions |
| .github/workflows/build.yml | Minor whitespace cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print(f"\nGenerating {total_commits} commits...") | ||
|
|
||
| # Use multithreading to process commits | ||
| max_workers = args.threads if hasattr(args, "threads") else args.max_threads |
There was a problem hiding this comment.
The line references args.threads which is never defined in the argument parser. The parser only defines --max_threads (which maps to args.max_threads). The hasattr check will always be False, so this fallback is unnecessary and could be confusing. Remove the hasattr check and directly use args.max_threads.
| max_workers = args.threads if hasattr(args, "threads") else args.max_threads | |
| max_workers = args.max_threads |
| with git_lock: | ||
| with open(os.path.join(os.getcwd(), "README.md"), "a") as file: | ||
| file.write(message(date) + "\n\n") | ||
| run(["git", "add", "."]) | ||
| run( | ||
| [ | ||
| "git", | ||
| "commit", | ||
| "-m", | ||
| '"%s"' % message(date), | ||
| "--date", | ||
| date.strftime('"%Y-%m-%d %H:%M:%S"'), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Using a global lock for all git operations serializes the entire commit process, which defeats the purpose of multithreading. While git operations need to be thread-safe, wrapping the entire contribute function in a lock means only one commit can be processed at a time, providing no performance benefit from multithreading. Consider using a more granular locking strategy or documenting that the performance gains claimed in the README may not be realized with this implementation.
| for commit_time in commits_to_make | ||
| ] | ||
|
|
||
| with tqdm( # type: ignore[misc] |
There was a problem hiding this comment.
The type ignore comment for tqdm is unnecessary. The tqdm function is properly typed and should not require ignoring type checking. Remove this type ignore comment unless there's a specific mypy error that needs to be addressed.
| with tqdm( # type: ignore[misc] | |
| with tqdm( |
| try: | ||
| future.result() | ||
| pbar.update(1) | ||
| except Exception as e: | ||
| print(f"\nError creating commit: {e}") |
There was a problem hiding this comment.
The error handling silently continues after printing an error, which could lead to incomplete repository generation without clear indication of failure. Consider either collecting errors and reporting them at the end, or failing fast when errors occur, depending on the desired behavior.
| for future in as_completed(futures): | ||
| try: | ||
| future.result() | ||
| completed += 1 | ||
| progress = (completed / total_commits) * 100 | ||
| print( | ||
| f"\rProgress: {completed}/{total_commits} commits ({progress:.1f}%)", | ||
| end="", | ||
| flush=True, | ||
| ) | ||
| except Exception as e: | ||
| print(f"\nError creating commit: {e}") |
There was a problem hiding this comment.
The error handling silently continues after printing an error, which could lead to incomplete repository generation without clear indication of failure. Consider either collecting errors and reporting them at the end, or failing fast when errors occur, depending on the desired behavior. This duplicates the same issue in the tqdm branch.
| days.""", | ||
| ) | ||
| parser.add_argument( | ||
| "-t", |
There was a problem hiding this comment.
The short form argument -t is used for --max_threads, but this conflicts with common conventions where -t often stands for "test" or "target". Consider using a different short form like -mt to better align with the long form --max_threads and avoid potential confusion.
| "-t", | |
| "-mt", |
| > [!NOTE] | ||
| > The progress bar requires `tqdm` (optional dependency). Install it for the best experience: | ||
| ```sh | ||
| pip install tqdm | ||
| # OR | ||
| pip install -r requirements.txt | ||
| ``` | ||
|
|
||
| **Without tqdm:** The script works perfectly fine without it! Fallback to simple progress indicator instead |
There was a problem hiding this comment.
The note about the progress bar not requiring tqdm contradicts the earlier statement. Line 63 says "The progress bar requires tqdm" but line 71 says "Without tqdm: The script works perfectly fine without it! Fallback to simple progress indicator instead". Consider clarifying that tqdm is optional but provides an enhanced progress bar, while a basic progress indicator is used as a fallback.
| if TQDM_AVAILABLE: | ||
| # Use tqdm progress bar | ||
| with ThreadPoolExecutor(max_workers=max_workers) as executor: | ||
| futures = [ | ||
| executor.submit(contribute, commit_time) | ||
| for commit_time in commits_to_make | ||
| ] | ||
|
|
||
| with tqdm( # type: ignore[misc] | ||
| total=total_commits, desc="Creating commits", unit="commit" | ||
| ) as pbar: | ||
| for future in as_completed(futures): | ||
| try: | ||
| future.result() | ||
| pbar.update(1) | ||
| except Exception as e: | ||
| print(f"\nError creating commit: {e}") | ||
| else: | ||
| # Fallback to simple progress indicator | ||
| completed = 0 | ||
| with ThreadPoolExecutor(max_workers=max_workers) as executor: | ||
| futures = [ | ||
| executor.submit(contribute, commit_time) | ||
| for commit_time in commits_to_make | ||
| ] |
There was a problem hiding this comment.
The code for submitting futures to the ThreadPoolExecutor is duplicated between the tqdm and non-tqdm branches (lines 75-78 and 93-96). Consider extracting the future submission logic before the conditional to reduce duplication and improve maintainability.
| try: | ||
| from tqdm import tqdm | ||
|
|
||
| TQDM_AVAILABLE = True | ||
| except ImportError: | ||
| TQDM_AVAILABLE = False | ||
| print("Note: Install 'tqdm' for a better progress bar experience: pip install tqdm") | ||
|
|
||
| # Global lock for git operations to avoid conflicts | ||
| git_lock = Lock() | ||
|
|
||
|
|
||
| def main(def_args=sys.argv[1:]): |
There was a problem hiding this comment.
Print statement may execute during import.
| try: | |
| from tqdm import tqdm | |
| TQDM_AVAILABLE = True | |
| except ImportError: | |
| TQDM_AVAILABLE = False | |
| print("Note: Install 'tqdm' for a better progress bar experience: pip install tqdm") | |
| # Global lock for git operations to avoid conflicts | |
| git_lock = Lock() | |
| def main(def_args=sys.argv[1:]): | |
| _TQDM_WARNING = None | |
| try: | |
| from tqdm import tqdm | |
| TQDM_AVAILABLE = True | |
| except ImportError: | |
| TQDM_AVAILABLE = False | |
| _TQDM_WARNING = "Note: Install 'tqdm' for a better progress bar experience: pip install tqdm" | |
| # Global lock for git operations to avoid conflicts | |
| git_lock = Lock() | |
| def _print_tqdm_warning(): | |
| """Print a notice about installing tqdm, when appropriate.""" | |
| if _TQDM_WARNING: | |
| print(_TQDM_WARNING) | |
| def main(def_args=sys.argv[1:]): | |
| _print_tqdm_warning() |
Scott-Nx
changed the title
1 participant
Performance and Usability Enhancements:
contribute.py, allowing commits to be generated in parallel using a configurable number of threads for a 2-4x speeduptqdmfor real-time feedback during commit generation, with a fallback to a simple progress indicator iftqdmis not installedDocumentation Updates:
.github/README.mdby add multithreading, progress bar support, installation instructions fortqdm, new command-line options and improved formattingCommand-line Interface Improvements:
--max_threadsargument to control the number of parallel threadsTesting Improvements:
test_contribute.pyto use the new style format.